mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 22:13:18 +09:00
3d4e8c07e99d01c74ca99fec88a307bc6bf4adbe
5509
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Aleksei Sviridkin
|
3d4e8c07e9 |
test(tenant): anchor the operator-gate absence and refresh a spent reason (#3661)
## What this PR does Two tests in `packages/apps/tenant/tests/vmo_webhook_dependson_test.yaml` say the seaweedfs and gateway HelmReleases deliberately carry no operator gate, and assert that with `notExists: spec.dependsOn` and nothing else. A bare absence assert also passes when the template renders no document at all, so both stayed green with those two releases switched off entirely. With `seaweedfs: false, gateway: false` the suite still reported 5 of 5 passing; pinning `metadata.name` next to the absence turns the same run into 2 failures. An earlier check on these asserts injected a `dependsOn` and watched them redden. That proves they catch an added gate, which is the other direction. For a negative assert the direction that matters is the document going away, and that one was never covered. Pairing `equal: metadata.name` with the absence is not a new idiom introduced here. `no_upgrade_force_test.yaml`, in the same directory, already anchors all seven of its releases that way, and this brings the last two suites in the chart into line with it. Both suite headers also carried a reason for their `templates:` list that has expired: that rendering the whole chart fails because `keycloakgroups.yaml` indexes `.Values._cluster` unguarded. #3636 added the guard, so a no-values render of the chart now succeeds. The list is still load-bearing, for the reason `no_upgrade_force_test.yaml` already records — it keeps an unrelated template breaking out of this suite's result. Checked by adding a nil dereference to a template neither suite asserts on: both suites stay green with their lists and error without them. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. It touches two files, both under `packages/apps/tenant/tests/`, and no chart, template, values file or tooling. Nothing in the map keys on test files. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ``` |
||
|
Aleksei Sviridkin
|
340483fdf0 |
fix(hack): declare the shared package targets and the root test target phony (#3353)
## What this PR does `hack/package.mk` set `.PHONY` with `=` instead of `:`: ```make .PHONY=help show diff apply delete update image ``` That assigns a variable which happens to be named `.PHONY`; it declares nothing. `make -p` prints it back among the variables (`.PHONY = help show diff apply delete update image`) rather than as a special target, so none of the targets `package.mk` defines was phony in any package that includes it. Nothing in the tree collides with those names today, so this is preventive rather than a fix for present breakage. The failure it forecloses is quiet: with a directory named `image` beside a package Makefile, `make image` prints `make: 'image' is up to date.` and exits 0 without building. A file of that name is enough on its own where the rule has no prerequisites; where it has one, the recipe keeps running until that prerequisite is shadowed too. The list changed as well. `suspend`, `resume`, `check` and `clean` are defined in this file and were missing, and `check` is the one that matters most, since it is the prerequisite that keeps `show`, `diff`, `apply` and `delete` rebuilding. `update` and `image` are gone, because `package.mk` does not define them; the including package Makefile does. Naming an undefined target in `.PHONY` creates it as an empty target, turning `No rule to make target` (exit 2) into `Nothing to be done` (exit 0) wherever the rule is missing. That reaches CI too: `hack/build-matrix.sh` parses the root Makefile `build:` target and runs `make -C <pkg> image` per unit, so a package arriving in that list without an `image:` rule would go from a red job to a green one that builds nothing. `.DEFAULT_GOAL` on the line above keeps its `=`. That one is a real variable, so the assignment form is right there. A comment now says which of the two lines is which, since they read alike and one of them just changed shape. `hack/package-mk-phony.bats` covers the declaration and is picked up automatically by the `hack/*.bats` glob in `bats-unit-tests`. Five of its six tests cover `package.mk`: that the declared names are really phony with a file of each name sitting beside the Makefile; a control that mutates a copy back to the assignment form and requires the probe to report up-to-date, so the first cannot pass vacuously; that the declared list and the defined targets are the same set, checked both directions; and two pinning the `update`/`image` exclusion, one requiring the loud failure to survive and one extending the list in a copy and requiring it to disappear. The loud-failure test pins both the exit status 2 and the diagnostic. A wrapper that kept make's wording while returning something else would pass a check on the message alone. One point decides where the guarantee actually lives. The first test reads its names from the `.PHONY:` line, so it cannot see a name dropped from that line at all. The set-equality test is what guards every name's presence. One known gap is left in place. The first test decides by matching make's `is up to date`, which make prints only for a target that has a recipe, and a recipe-less alias never draws it: with no prerequisite both forms print `Nothing to be done`, and with a prerequisite that is itself out of date, which every target in this file is for being phony, the working form prints that prerequisite's recipe while the broken form prints `Nothing to be done`. The probe separates neither pair, so such a name would pass the first test either way. Measured on both shapes, not assumed. Every target `package.mk` defines has a recipe, so nothing is uncovered today; the control test does not close the gap either, since an alias is a defined target and the control requires `is up to date` from it, which turns it red against the probe rather than against the alias. Worth closing if an alias is ever added. One more for whoever edits the suite next: `hack/cozytest.sh` rewrites every bare `}` at column 0 into `return 0` followed by `}`. That is aimed at `@test` blocks, but it does not distinguish them from a plain shell function, so a helper factored out of these tests would silently become unable to return a non-zero status. Tests 1 and 2 repeat a loop rather than share one for reasons of their own, but this is the trap waiting for anyone who merges them. Two properties of the suite are worth stating, because both are easy to get wrong in the other direction. The set-equality test extracts the defined targets with a pattern that accepts a rule naming several targets at once, so `suspend resume: check` keeps both names in the set; anchoring the colon to a single name drops every name on such a line, and for a target nothing else depends on that goes unnoticed. And the control test probes the targets `package.mk` defines rather than the names on its `.PHONY:` line, because `is up to date` is only a meaningful answer for a name that has a rule: a declared-but-undefined name prints `Nothing to be done` in both the working and the broken form, so probing it in the control fails against the probe, while the set-equality test holds the message that names the defect. Last one, about how the tests find the repo. All six read it as `repo=$(pwd)`, which assumes the runner starts at the repo root. That holds for `make bats-unit-tests` and for the invocation the header documents, so nothing is broken, and the assumption is the ordinary shape in `hack/` rather than an outlier: about half the bats files there resolve repo-relative paths against the cwd the same way, several stating it in their own headers, and this is not even the only one spelling it with `$(pwd)`. What is worth naming is the failure mode: started elsewhere, these tests go red with `no '.PHONY:' declaration found in hack/package.mk`, which reads like a real finding rather than a wrong directory. The root `Makefile` had the same defect, and there it had already fired. `test` was not on its `.PHONY:` line and a `test/` directory sits beside it, so `make test` printed `'test' is up to date` and exited 0 without running anything, while `docs/agents/overview.md` advertises that command as the way to run the full suite. CI never used it: the workflow calls `test-controllers` and drives e2e out of `packages/core/testing`, so the cost fell on anyone following the docs by hand. One name on the declaration closes it, and the suite gained a sixth test that goes red without it. Every other undeclared target in that file is unshadowed today, so none of them is reachable the way `test` was, and they are left alone here. #3709 covers the same class in the four `packages/core/` Makefiles, which include only `hack/common-envs.mk` and so are out of reach of this change entirely. ### Downstream repositories `scripts/package.mk` in cozystack/external-apps-example was a byte-for-byte copy of `hack/package.mk` (identical sha256 before this change), carrying the same inert `.PHONY=` line. This PR splits the two files, and the copy was fixed separately. It landed the same nine names, and `update` and `image` are absent there for the reason they are absent here: `scripts/package.mk` does not define them either, and neither does any Makefile that includes it. What separates the two files now is the two-line comment added here, and nothing else. - [ ] 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: - [x] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: https://github.com/cozystack/external-apps-example/pull/4 - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed task declaration handling so supported commands are correctly recognized and executed. * Updated available commands by replacing `update` and `image` with `suspend`, `resume`, `check`, and `clean`. * **Tests** * Added coverage to verify command declarations, prevent regressions, and ensure invalid or missing commands report errors correctly. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
d978a371a2 |
test(hack): cover the .PHONY declarations this tree relies on
`.PHONY=a b c` and `.PHONY: a b c` differ in the `=` against the `:`, and only the second declares anything. The first defines an ordinary variable named `.PHONY` that nothing reads, leaving every listed target file-backed. A file or directory of that name beside a package Makefile then stands in as the product of the rule: for a target with no prerequisites make reports it up to date, skips the recipe and exits 0, and a target whose prerequisite is itself file-less keeps rebuilding until that prerequisite is shadowed too. The neighbouring `.DEFAULT_GOAL=help` is a real variable and is correct with `=`, so the two lines read alike while only one of them works, which is what let the inert form sit there unnoticed. Add hack/package-mk-phony.bats, six tests, picked up automatically by the hack/*.bats glob in `bats-unit-tests`. Three cover the declaration itself. The first reads the names off the `.PHONY:` line rather than repeating them, plants a file named after each beside a stub Makefile, and requires `make --dry-run` to print the recipe instead of "is up to date". A negative control mutates a copy back to the assignment form and requires the probe to report up-to-date, over the targets package.mk defines rather than the names it declares, because "is up to date" is only a meaningful answer for a name that has a rule. The third keeps those two sets equal: it requires the declared list and the defined targets to be the same set. Defined but undeclared is the direction the list rots in: check, clean, suspend and resume sat undeclared for as long as the declaration itself was inert. Declared but undefined is the empty-target trap in its general form, so a future name cannot slip in the way update and image once did. Two more pin what the list deliberately leaves out. package.mk defines neither `update` nor `image`; the including package Makefile does, and most packages define neither. Naming them here would create them as empty targets everywhere, turning "No rule to make target" into "Nothing to be done". Since the CI build matrix is parsed from the root Makefile `build:` target and runs `make -C <pkg> image` per unit, a package reaching that list without an `image:` rule would go from a red job to a green one that builds no image. One test requires that loud failure to survive, pinning both of the observables it is made of: make's exit status 2 and its diagnostic. They are independent, and a wrapper that kept the wording while returning something else would satisfy a check on the message alone. Its control extends the declaration with the two names in a copy and requires the failure to disappear, so the pin tracks the list rather than some property of the harness. Every decoy carries one fixed timestamp. Six of the nine targets take check as a prerequisite, and apply and delete reach it through suspend as well, so touching the decoys in list order can leave check newer than its own dependents. Make then rebuilds them for that reason and prints a recipe whatever .PHONY says, and the probe reports success against a declaration that does nothing. Equal mtimes remove the question in any list order. Measured on GNU Make 4.3: list order left the per-target assertion inert for up to six of the nine, moving with where the clock tick fell, while 3.81 fits the loop inside a single timestamp and shows none of it. The control test plants its decoys the same way for the same reason, and goes red if that is undone in its own loop. The two loops are independent, so neither guards the other, and the header is what keeps the arrangement from being unpicked. A sixth test leaves package.mk and covers the root Makefile, where this class had already bitten: `test` is the one target in the tree with a path of its own name beside it. It probes with --dry-run because the recipe wants a live cluster, and fails loudly if that path ever goes away rather than passing on a question with no subject. `%-update` is not coverable: .PHONY matches target names literally and does not expand patterns. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
IvanHunters
|
09dc7f47fb |
feat(backups): add a native MongoDB (psmdb) backup-strategy driver (#3562)
## What this PR does Adds a native MongoDB (Percona Server for MongoDB / psmdb) backup-strategy driver so `apps.cozystack.io/MongoDB` reaches feature parity with the CNPG, MariaDB and ClickHouse native drivers — logical backups, restore into a differently-named instance, and point-in-time recovery — all through the unified `BackupJob` / `RestoreJob` / `BackupClass` / `Plan` interface (closes #3545). The driver orchestrates the operator's `PerconaServerMongoDBBackup` / `PerconaServerMongoDBRestore` CRDs: - **BackupJob**: creates a one-shot `PerconaServerMongoDBBackup`, polls `status.state`, and persists the S3 destination + storage descriptor (by reference — Secret name only, never a raw credential) onto the Cozystack `Backup` artifact. - **RestoreJob**: drives `PerconaServerMongoDBRestore` via `backupSource` (read back from the source backup status, with a snapshot fallback for a reaped operator CR) so the same artifact restores in-place **or** into a differently-named instance. PITR is exposed as `spec.options.recoveryTime` (RFC3339), matching the CNPG driver's option exactly; unrecognised `spec.options` keys are surfaced as a `UnknownRestoreOption` Warning event rather than silently dropped. Because psmdb resolves the backup storage by name against the cluster's `spec.backup.storages`, the driver names the chart's storage (`s3-storage`) via the `cozy-default-mongodb` Strategy rather than injecting it, and surfaces a clear `Ready=False` precondition when a target has backups disabled instead of hanging until the deadline. **Wiring** - `api/backups/strategy/v1alpha1`: `MongoDB` strategy type + generated CRD/deepcopy. - `internal/backupcontroller`: `psmdbstrategy_controller` + `psmdbtypes` / `mongodbapp` partial-schema mirrors; BackupJob/RestoreJob dispatch, supported-kinds, cleanup-on-delete branch, and scheme registration. - `backupstrategy-controller`: `cozy-default` MongoDB route, `cozy-default-mongodb` Strategy CR, and `psmdb.percona.com` RBAC (get/list/watch, +create on backups/restores, no delete — the driver does not own the S3 archive). - `apps/mongodb`: a `backup.insecureSkipTLSVerify` value (psmdb's s3 storage has no CA-bundle field) so backups can target Cozystack's self-signed in-cluster seaweedfs. Default `false`, fully backward-compatible; regenerated schema/README/RD. **Tests + e2e** - Unit tests for the driver: applicationRef validation, ensure-idempotency (backup + restore), the backups-enabled/storage precondition gate, `backupSource` resolution (live-preferred / snapshot fallback / no-destination), snapshot round-trip, the producer-seam (`createMongoDBBackupArtifact`) driver-metadata, the cleanup-on-delete no-Velero-fall-through guard, and `recoveryTime` → psmdb-UTC mapping + unknown-option reporting. - `examples/backups/mongodb/` + a `mongodb-2-backup-roundtrip` Chainsaw suite that drives `run-all.sh` end-to-end (a sentinel document round-trips through S3 into a restored copy while the source stays untouched), per `docs/agents/e2e-testing.md`. - `docs/operations/backup-classes.md` updated (new route, the app-owns-the-storage model, and the `recoveryTime` PITR usage). Verified end-to-end on a live dev cluster against the platform-default `cozy-backups` bucket: backup via `cozy-default` reached the bucket and the Cozystack `Backup` went `Ready`, and a to-copy `RestoreJob` restored the sentinel into a differently-named instance with the source left intact. ### Screenshots N/A — no UI changes (controller, chart values, CRD, docs and tests only). ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` file by file against the diff: - **cozystack/website** — the only app-facing change is `packages/apps/mongodb` (a new `backup.insecureSkipTLSVerify` value + regenerated `README.md`). The managed-app reference page is regenerated from the package `README.md` by the release bot for apps already in the website `Makefile` (mongodb is one), so it refreshes automatically on the next tag — no manual follow-up. `docs/operations/backup-classes.md` is not a website-mirrored doc in the trigger map. - **cozystack/terraform-provider-cozystack** — the one candidate trigger ("a field added to an app's `values.schema.json`") lands inside the mongodb `backup` block, which the provider's `internal/provider/mongodb_schema.go` explicitly states it does **not** manage ("The deprecated backup block, the bootstrap restore block, and detailed shardingConfig are not managed"). No enum, default, `release.prefix`, Secret/Service name, or hand-typed CRD (`Plan`, `RestoreJob`) changed; the new `strategy.backups.cozystack.io/MongoDB` CRD is opaque to the provider. Not affected. - All other repositories — no matching trigger (no `hack/` layout or make-target change, no namespace rename, no `ApplicationDefinition` semantics change, no `packages/core/platform` values, no telemetry metric or `cozy-proxy` label rename). - [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 native MongoDB (psmdb) backup-strategy driver — logical backup to the cozy-default BackupClass, restore into a differently-named instance, and point-in-time recovery via spec.options.recoveryTime ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added MongoDB backup and restore support, including scheduled and on-demand backups, restores to new or existing clusters, and point-in-time recovery. * Added configurable S3 TLS certificate verification for MongoDB backups, defaulting to secure verification. * Added MongoDB support to the default backup configuration. * **Documentation** * Added MongoDB backup and restore examples, workflows, and operational guidance. * **Tests** * Added automated coverage for MongoDB backup, restore, cleanup, and end-to-end scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
61a236cd96 |
feat(ci): auto-apply triage labels to issues (#3574)
## What this PR does Issues that arrive through the API or `gh issue create` bypass the issue templates, so they carry no `triage/*` label and nothing in the repository adds one afterwards. Listing every open issue through the same endpoint the workflow uses, `gh api --paginate "/repos/cozystack/cozystack/issues?state=open&per_page=100"`, returns 567 entries, 171 of them pull requests, at the time of writing. That leaves 396 open issues, and 252 of them carry no triage label at all. Every count in this description is a point-in-time reading of a backlog that keeps moving, so a later run of the same query will differ by a few. The triage queue is impossible to filter on in that state, and there is no way to see how much of the backlog nobody has looked at. This adds a workflow that labels an issue when it is opened or reopened, plus a daily sweep that backstops labels removed by hand and works through the existing backlog. An issue somebody has assigned, or marked with one of `priority/critical-urgent`, `priority/important-soon`, `priority/important-longterm` or `epic`, gets `triage/accepted`, since a maintainer has already looked at it. Everything else gets `triage/needs-triage`. An issue already carrying any `triage/*` label is left alone, so the sweep never overwrites a decision someone made by hand, and pull requests returned by the same listing endpoint are skipped. Against the listing above, the sweep applies `triage/accepted` to 39 issues and `triage/needs-triage` to 213, and touches none of the 144 that are already triaged. Deleting a triage label by hand does not undo any of this. An issue left carrying no `triage/*` label is relabeled by the next sweep, so a triage decision is changed by replacing the label. `AGENTS.md`'s labeling section now says so, because that is the file somebody reads before labeling by hand. Two conditions decide what counts as a signal. Membership of `stale.yaml`'s `exempt-issue-labels` is necessary: `triage/accepted` is itself on that list, so applying it does more than reset a clock, the issue stops being auto-closable, and a signal that is not already exempt would hand out that permanent reprieve as a side effect of being labeled. `priority/backlog`, the one tier `stale.yaml` deliberately leaves reapable, is excluded for exactly that reason. Membership is not sufficient, which is why the signal list is not simply the exempt list. `lifecycle/frozen` is exempt and still not a signal: a `lifecycle/*` label states what the stale bot may do with an issue and says nothing about whether anybody reviewed it. Freezing an untriaged issue to keep the bot off it is the case where reading it as accepted is wrong. Of the five open issues whose only signal would have been `lifecycle/frozen`, one is blocked on an upstream project and carries `help wanted`, and one is an older feature request that an open epic now covers. Neither is ready to be worked on, and stamping either accepted would have kept it out of triage for good, since the sweep never revisits an issue that already carries a triage label. `epic` passes the same test from the other side: it is a maintainer-authored planning artifact rather than an inbound report, and all eight open issues whose only signal is `epic` are roadmap trackers that `needs-triage` would describe falsely. A contract test pins the rule and not the instance, so no label from the `lifecycle/*` namespace can quietly become a signal later. `security/*` stays out on the same grounds, though the case is closer: those labels record what is known about a vulnerability rather than whether anyone triaged the report, and the cost of being wrong runs the cheap way, since a `needs-triage` on a confirmed report is one click to remove while a wrong `accepted` is permanent because the sweep never revisits. One caveat stated plainly: the signals stand for a maintainer decision already recorded on the issue, which is a weaker claim than the one `triage/accepted` makes in `.github/labels.yml`, and `priority/important-longterm` says outright that the work may not be staffed. Of the two labels this workflow can write it is still the closer one, since `needs-triage` on an issue somebody already prioritised or took asks for a triage that has happened, and a maintainer who disagrees replaces the label in one click. An assignee is the single signal that is neither a label nor already exempt, so an issue whose only signal is an assignee does gain a permanent reprieve. That is the intended reading rather than a side effect: somebody took ownership, which makes it a known long-tail task rather than an abandoned one, and it is the same argument `stale.yaml`'s own comment makes for exempting `triage/accepted` in the first place. Three operational details. First, the sweep paces its writes a second apart and caps a run at 400, because GitHub allows 80 content-generating requests a minute and 500 an hour, this client carries no throttling plugin and no retries, and a throttled write answers `403`, which is exempt from retry anyway. Unpaced, the first pass over the backlog would not slow down when throttled, it would fail the job partway and leave the backlog half labeled. Whatever a run leaves behind stays unlabeled, so the next one picks it up. Second, adding a label bumps an issue's `updated_at`, which `stale.yaml` reads as activity, so the first sweep restarts the 60-day staleness clock on every issue it writes to, all 252 of them, and un-marks the 32 that carry `lifecycle/stale` today. Both are deliberate and one-time: the backlog gets triaged now and ages again from there, rather than being closed unread while it is still unlabeled. Third, a run writes a job summary with the counts, the issue numbers under each label, and both remainders it can leave behind: the write cap's deferral, and how much of the listing it never reached when a refusal stopped it early. A sweep over the backlog logs a line per issue, and the summary is what makes the outcome readable without paging through the 252 the current backlog produces. A write that fails does not take the rest of the run with it, but the two failure shapes are told apart rather than counted together. Any status other than a `403` or `429` lets the run carry on, most often a `404` or `410` from an issue transferred or deleted mid-sweep, since a failure about one issue says nothing about the next. A `403` or `429` means the token lost `issues: write` or the pacing stopped being enough, which applies to every write left in the run, so the sweep stops at the first one and the job fails loudly. Repeating a rejected request up to the cap is what gets an integration throttled harder. `workflow_dispatch` takes a `dry_run` input that logs what would be labeled without writing anything. It is on by default, so a manual run writes nothing until somebody unchecks the box, and `schedule` is the one event that writes without being asked. The cap applies in dry-run too, so a dry run predicts what a real one would do instead of printing a longer list nobody gets. The job asks for `issues: write` and everything else is read-only, the action it uses is pinned by digest, and a 20-minute timeout keeps a hung call from holding the queue for the default six hours. No pull request lane can exercise this workflow, so its first real execution is a sweep over every open issue with nothing having run it before. `hack/issue-triage-contract.bats` pins the executable lines that bound that blast radius: the pull-request skip, the already-triaged skip, the pacing rate and the write cap, the job timeout, the failure handling, the digest-pinned action, the token scopes, the serialised queue, the dry-run default and the derivation that does not depend on it, the cron ordering against `stale.yaml`, that both labels it writes exist in `.github/labels.yml`, that no accepted-signal label sits outside `stale.yaml`'s exempt list or inside the `lifecycle/*` namespace, and that the summary is written with the `await` that makes it flush and accounts for every outcome a run can end on. Each assertion was checked against a mutated copy of the workflow to confirm it goes red rather than merely existing, with the mutations run through `hack/cozytest.sh` rather than `bats`. The two runners disagree about negated assertions, so a pin that `bats` reports as live can be dead under the runner CI actually uses, and every negative assertion in the file captures grep's status instead of leading with `!`. Suggested first step after merge: run the workflow by hand and read the summary before the 05:53 cron arms itself. The box arrives checked, so that run costs nothing. ### Screenshots Not a UI change, so there is nothing to show. ### Downstream repositories I walked the trigger map in `docs/agents/contributing.md` against the diff. The diff adds two files, `.github/workflows/issue-triage.yaml` and `hack/issue-triage-contract.bats`, and appends one line to `AGENTS.md`. No row matches: it adds and renames no package, changes no chart values, no schema, no version enum, no `ApplicationDefinition`, no CRD, no namespace, no variant or bundle, no label or annotation any downstream repository matches on, no metric and no release asset. Its `hack/` rows name `hack/upload-assets.sh`, `hack/e2e-prepare-cluster.bats`, `hack/package.mk`, `hack/update-crd.sh` and `hack/common-envs.mk`, which downstream repositories copy, anchor on or mirror by hand, plus one row for moving or renaming anything under `hack/`; a new bats file added next to them is none of those. The map has no row for the agent documentation, and no downstream repository copies `AGENTS.md`. - [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(ci): new and reopened issues now get a `triage/*` label automatically, and a daily sweep labels any open issue that has none: `triage/accepted` when it is assigned or carries an `epic` or a priority above `priority/backlog`, `triage/needs-triage` otherwise. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added automated issue triage triggered by new or reopened issues, scheduled reviews, or manual runs. * Issues can be labeled as needing triage or accepted, with pull requests and already-triaged items excluded. * Added dry-run support and reporting for triage results. * **Documentation** * Added guidance for replacing existing triage labels when making decisions. * **Tests** * Added coverage for workflow behavior, permissions, scheduling, failure handling, and safety limits. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
59878850f9 |
test(e2e): capture worker CPU throttling counters on node-join failure (#3723)
<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->
## What this PR does
When a tenant worker misses the node-Ready deadline, the failure bundle
currently cannot tell two different causes apart: a worker held at its
own CFS ceiling, and a worker losing host CPU it was entitled to. Both
look identical from outside the Pod, and node-level utilisation answers
neither, because it shows what the node used rather than what the
container was allowed.
This adds a collector that reads the CFS counters and the ceiling itself
from the kubelet's cAdvisor endpoint, for every tenant worker on every
node in the sandbox. The counters are the only place that difference is
recorded, and nothing else in the tree collects them for these workers.
The motivating measurement is on nightly run 31290999592: the `compute`
container reports 0.968 and 0.920 core against a `limits.cpu` of 1,
which is 92-97% of a one-core ceiling. Whether the container ever
actually met that ceiling is exactly what the artifact cannot say, and
what these counters answer.
Most of the change is the failure taxonomy rather than the read. An
empty capture is the dangerous outcome here, because zero bytes reads as
a container that never hit its ceiling -- the conclusion the collector
exists to stop a reader reaching by default. So every way the capture
can come up short gets its own sentence: the kubelet was not read, the
read was cut short part way, the metric stream could not be read back on
this runner, the kubelet answered and carried no series for this
namespace, the namespace was there and the worker Pods were not. The one
non-failure outcome is written down too: a container with no CPU limit
puts a single line on the wire, which is also what a truncated read
leaves behind, so it says which one it is.
### Ordering
The collector runs first among the gated collectors, ahead of the guest
serial console. Its ceiling is four bounded reads, about 100s of a 480s
phase budget, so on a slow run the console capture starts that much
later. That is a deliberate trade and not a side effect of putting the
cheapest first: cost is not what settles the order, survivability of the
answer is. The console evidence cannot be re-taken, and this collector's
question has no other answer in the tree, so both go ahead of the
captures whose state is partly recoverable from reads above them.
### Known limits, named rather than left to be found
- The outcome ladder enumerates combinations of read status, filter
status and result emptiness. Every new source of partial output needs a
new rung, and that has already happened twice while this branch was in
review. Deriving the label from the three axes instead would make a new
source land in an existing axis.
- The uncapped note is decided over the whole per-node file, so one
quota line anywhere in it suppresses the note. That is the conservative
direction -- it never claims uncapped over a file that shows a ceiling
-- but it makes the note rare.
- The messages say `see read-error.log` while the file on disk is
`<node>.read-error.log`. That is the convention the neighbouring
serial-console collector already uses, and changing one side alone would
make the two inconsistent.
- The `mkdir -p` for the report directory is unchecked, as it is in four
neighbouring collectors in the same file. Worth fixing across the file
rather than in one collector.
- The section letters in the failure block no longer track emission
order. They already did not before this change; this adds one more entry
to the sequence.
- The count of bats files sourcing the library, in
`docs/agents/e2e-testing.md`, is corrected here from three to seven.
Nothing pins it, so it will drift again.
- The node walk iterates `for node in ${nodes}` unquoted, so pathname
expansion applies alongside word splitting. Node names are RFC1123 so it
cannot bite today; `while IFS= read -r node` would make that structural
instead of incidental.
### Screenshots
<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->
### Downstream repositories
<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.
Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.
IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:
1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->
- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
### Release note
<!-- Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->
```release-note
test(e2e): capture tenant worker CPU throttling counters and their CFS ceiling when a node-join fails, so a worker held at its own limit can be told apart from one losing host CPU
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Added worker CPU-throttling diagnostics to Kubernetes troubleshooting
reports.
* Reports now identify CPU quota gaps, partial reads, filtering issues,
and incomplete results.
* Added safeguards for bounded node traversal and configurable
collection budgets.
* **Bug Fixes**
* Improved failure handling when diagnostic commands, timeouts, or
metric reads are unavailable.
* **Tests**
* Expanded coverage for CPU-throttling capture, node-join diagnostics,
timeout behavior, configuration validation, and partial failures.
* **Documentation**
* Updated diagnostic collector counts and collection-order
documentation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Aleksei Sviridkin
|
3ad4d452fc |
fix(tests): fail when a chart runs no Helm unit test suites (#3632)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does `helm unittest` is fail-closed on a suite it cannot parse and on one declaring `tests: []`, and fail-open on suites that are not there at all: it prints `Test Suites: 0 passed, 0 total` and exits 0. `hack/helm-unit-tests.sh` judges each package by that exit code alone, so a chart whose suites were deleted, moved, or renamed past the `tests/*_test.yaml` glob reports success having asserted nothing. That is measurable in both directions rather than argued. Move `packages/system/velero/tests/velero_test.yaml` aside and run the script as it exists on main today: ```text Running tests in packages/system/velero helm unittest . ### Chart [ cozy-velero ] . Charts: 1 passed, 1 total Test Suites: 0 passed, 0 total Tests: 0 passed, 0 total ``` The run ends with `All Helm unit tests passed.` and exit 0. The chart passed, and the count of things it checked was zero. With this PR the same tree exits 1 and names the package and the reason. Neither `--strict` nor an explicit non-matching `-f` glob changes this on its own; all three forms exit 0, checked against plugin 1.1.1. So the script now reads the captured output and fails the package when the zero-suite line appears. Matching that line names a cause and a remedy where it applies, but the set of ways to run nothing is not closed. A recipe that never invokes `helm unittest`, and a `test:` rule that make finds nothing to do for, print no marker of any kind and exit 0, so no additional match reaches them; enumerating absences leaves a fresh hole every time a new shape turns up. So the script also requires the line a real run always emits, a `Test Suites:` count of at least one. That turns the question round: every way of asserting nothing exits 0, and "what did this run assert" is the question that has an answer. All 76 packages the script visits emit that line today. The tradeoff is that a `test` target which legitimately runs no `helm unittest` would need renaming or an opt-out, which is the same bargain the zero-suite check already strikes. The discovery gate has the same shape from the other side. `make -C dir -n test` succeeds against a file-backed target, so a path named `test` beside a package Makefile whose rule is not phony would make both the gate and the run exit 0 without the recipe ever firing. The check keys on what make reports rather than on the path existing, because a package that declares the target phony runs its recipe whatever sits next to it, and refusing that would be a false failure. The quoting around the target name differs between make 3.x and 4.x, so it accepts either opening quote; anchoring on the trailing quote alone would also match a sub-make reporting some other target whose name ends in `test`. That second check is guarding a live gap rather than a hypothetical one. `hack/package.mk` line 2 reads `.PHONY=help show diff apply delete update image`, which is a variable assignment and not a target declaration, so it makes nothing phony. Of the packages the script visits that define a `test` rule, 18 declare it phony in their own Makefile; the rest are unprotected if a path of that name ever appears. No such path exists today. Fixing the shared include is #3353 and is not this PR. One deliberate non-change, stated so it is not rediscovered as an oversight. Running the suite now captures output instead of streaming it, because a pipeline's exit status in POSIX `sh` reports the last command rather than `make`. Streaming reads better and loses the status, so it stays buffered. The run is pinned to `LC_ALL=C`, since both checks match English wording and a localized `make` would disarm one of them silently, which is the failure mode the whole change exists to remove. The guard is all-or-nothing, which is worth stating plainly. A chart that loses four of its five suites still reports `Test Suites: 1 passed` and passes; only total disappearance is caught. Tying the expected count to what each chart actually has would be a per-chart number to maintain, and a number maintained in one place while the suites move in another is the failure this repo has been paying for elsewhere, so the cheap check that catches the total loss is the one worth having. One behaviour worth stating because the message does not: a suite in which every test carries `skip:` reports `Test Suites: 0 passed, 1 skipped, 1 total` and exits 0, and the positive-evidence check refuses it. That refusal is intended, since a wholly skipped suite asserted nothing, but the message talks about suites expected under `tests/`, which in that case are present and skipped on purpose. No package is in that state today. Partial skips are unaffected: `2 passed, 1 skipped, 3 total` satisfies the check. Three known gaps in what the change ships, none of them a wrong statement. The positive-evidence message explains its cause but stops short of naming a remedy, where the other two messages both end in an action; the remedy for the legitimate case, renaming the target or giving it an opt-out, currently lives only in the code comment. That same message enumerates two ways a run reports nothing, and the skipped-suite case above is a third, so it reads as a diagnosis where it is really a list of the common causes. And no document in the tree states the convention this tightens: `docs/agents/overview.md` still describes the script accurately as running over every package that defines a `test` target, but the contract is now that such a package must also report at least one passing suite. All three are improvements to make on the next touch of these files rather than reasons to hold the change. One adjacent gap stays open, and it is worth naming rather than leaving someone to assume otherwise. This catches a chart whose suite *files* vanished; it does not catch a package that loses its `test:` rule along with them. Nothing ties the presence of `tests/*_test.yaml` to the presence of a `test` target, so a change that removes both is skipped in silence, and the script only complains when no package in the tree has a `test` target at all. The test named `a package with no test target is skipped, not failed` pins that permissiveness deliberately, because the many packages that legitimately define no `test` rule would otherwise turn every run red. Closing it needs an instrument keyed on the suite files rather than on the Makefile target, which is a separate change. `hack/helm-unit-tests.bats` adds eight tests, picked up automatically by the `hack/*.bats` glob in `make unit-tests`. Three pin the new refusals, and each matches its own message so none can stay green on a refusal that came from another. Two pin what must not be refused: a phony target with a colliding path, and a sub-make reporting an unrelated target up to date. One pins that a chart which does run suites still passes, which is what stops the refusals being satisfied by rejecting everything. Two pin behaviour that was already there, namely that a failing package is still reported by name and that a package with no `test` rule is skipped rather than failed. That last one builds a tree carrying a second package that does run a suite: with only the target-less package present, the script takes an early exit above the failure summary and the assertion would hold whatever the script had done. The fixtures stub `make` output instead of invoking helm, so they need no plugin installed. Relates to #3453. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(tests): `make helm-unit-tests` now fails a chart that runs no Helm unit test suites, instead of reporting success for a chart whose suite files were deleted, moved, or renamed out of the `tests/*_test.yaml` glob ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Helm test validation to detect skipped test recipes, zero discovered suites, and runs without evidence of a passing suite. * Helm test failures now provide clearer diagnostics, including affected directories and failed commands. * Test output is consistently captured and replayed for easier troubleshooting. * **Tests** * Added comprehensive coverage for successful, failed, skipped, empty, and invalid Helm test scenarios. * Expanded coverage across package test targets, including phony and unrelated sub-make behavior. * **Documentation** * Clarified requirements for package test targets and suite reporting. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
3c009509ba |
test(cozystack-controller): guard the Secret grant by effective permission (#3627)
## What this PR does `packages/system/cozystack-controller/tests/rbac_test.yaml` has a test named "does not grant a blanket wildcard verb on secrets" that does not guard that. Its only assert is a `notContains` on `path: rules` holding a rule object. helm-unittest compares every field it is given by value equality, and `any: true` only changes which fields get compared, so nothing in that assert bounds what a verbs list may hold. `verbs: ["get", "*"]` passes every spelling of it. Each row below is a mutation of `templates/rbac.yaml`, confirmed visible in `helm template` output before I read any verdict. | # | widened Secret grant | whole suite | the test named as the guard | | --- | --- | --- | --- | | 1 | `verbs: ["*"]` on `""`/secrets, the exact object it names | red | red | | 2 | `resources: ["*"]`, `verbs: ["*"]` in the core group | red (positive pin) | green | | 3 | `apiGroups: ["*"]`, secrets, `verbs: ["*"]` | red (positive pin) | green | | 4 | `"*"` appended to the declared verb list | red (positive pin) | green | | 5 | the exact named object added as a second rule | red | red | | 6 | `verbs: ["*"]` plus a `resourceNames` field | red (positive pin) | green | | 7 | `deletecollection` spelled out | red (positive pin) | green | | 8 | declared rule kept, `apiGroups:['*'] resources:['*'] verbs:['*']` appended | green, 2 passed 2 total | green | | 9 | declared rule kept, `secrets: [deletecollection]` appended | green, 2 passed 2 total | green | Rows 8 and 9 are the bad ones. The declared rule stays untouched, so the positive pin is happy, the suite reports a clean pass, and the controller holds every verb on every Secret in every tenant namespace. The replacement is `hack/cozystack-controller-secret-rbac.bats`, which reads effective permission instead of manifest text. It renders the chart, unions the verbs every ClusterRole and Role rule confers on core Secrets, expands a `*` on any of the three axes, and compares that union against the declared set. All nine rows above are red under it, and so are widening the namespaced `Role`, bundling `secrets` with another resource under `verbs: ["*"]`, and dropping `delete` from the set. That last one is deliberate: the comparison is an equality rather than a ceiling, because both reconcilers need every verb in it and the controller is cluster-scoped, so a widening reaches every tenant. Reflowing the rule to block style changes the text and nothing else, and stays green. Green on `main` under both `bats` and `hack/cozytest.sh`. Expanding wildcards is what makes it hold at all. The chart ends with a blanket `apiGroups: ['*'] resources: ['*'] verbs: [get, list, watch]` rule that does reach Secrets, so a guard phrased as "no wildcard near secrets" would be red on `main` today. That rule predates this branch and is tracked separately in #3625. This guard stays green either way, whether the blanket rule is narrowed or left alone, as long as Secret access matches the declared set. The positive `contains` pin stays. Value equality is the right primitive for pinning a rule as written and the wrong one for bounding what the rule may become. The same shape exists in `packages/system/cozystack-api/tests/rbac_test.yaml` and `packages/extra/seaweedfs/tests/cleanup_client_test.yaml`, and more weakly in `packages/system/etcd-operator/tests/selector-fix-hook_test.yaml`, which matches a scalar inside `rules[0].verbs`. Out of scope here, listing them so they are not lost. `BATS_UNIT_FILES` picks the new file up on its own (`hack/*.bats`, no `e2e-` prefix), so `make unit-tests` runs it with no Makefile change. It needs helm, yq and jq, the same three `hack/admin-kubeconfig-invariant.bats` already needs. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff. The only entry that looks at `hack/` is cozystack/ccp, which triggers on moving or renaming something there, or on changing what a make target does. This adds a file and touches neither `hack/package.mk`, `hack/common-envs.mk`, nor any target. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ``` |
||
|
Aleksei Sviridkin
|
94c41d100f |
fix(hack): fail when a chart runs no Helm unit test suites
helm-unittest is fail-closed on a suite it cannot parse and on one declaring `tests: []`, and fail-open on suites that are not there at all: it prints "Test Suites: 0 passed, 0 total" and exits 0. Measured on plugin 1.1.1 against a throwaway chart, all three forms. hack/helm-unit-tests.sh judges each package by that exit code alone, so a chart whose suites were deleted, moved, or renamed past the tests/*_test.yaml glob reports success having asserted nothing. Moving packages/system/velero/tests aside reproduces it: the script prints "All Helm unit tests passed" and exits 0. Check the captured output for the zero-suite line and fail the package when it appears. Neither `--strict` nor an explicit non-matching `-f` glob does this on its own; both exit 0. Matching that line names the cause where it applies, but the set of ways to run nothing is not closed: a recipe that never invokes helm-unittest, and a `test:` rule make finds nothing to do for, both print no marker at all, so no further match reaches them. So also require the line a real run always emits, a `Test Suites:` count of at least one. Every way of asserting nothing exits 0, and asking what the run asserted is the question that has an answer. All 77 packages the script visits emit that line today. The tradeoff is that a `test` target which legitimately runs no helm-unittest would need renaming or an opt-out, which is the bargain the zero-suite check already struck. That is a contract contributors have to know about, so docs/agents/overview.md states it where it describes the sweep. Because the check matches helm-unittest's own summary wording, a plugin upgrade that reworded it would fail every package at once while blaming each package's Makefile. The error text says so, so the reader of a tree-wide red has the real cause in front of them rather than 77 copies of the wrong one. The discovery gate has the same shape from the other side. `make -C dir -n test` succeeds against a file-backed target, so a path named `test` beside a package Makefile whose rule is not phony would make both the gate and the run exit 0 without the recipe firing. Of the packages the script visits that define a `test` rule, 18 declare it phony, and no such path exists today. Match what make reports rather than looking for the path, because a package that declares the target phony runs its recipe whatever sits next to it, and refusing that would be a false failure. The quoting around the target name differs between make 3.x and 4.x, so the match accepts either opening quote; the trailing quote alone would also match a sub-make reporting another target whose name ends in "test". Running the suite now captures output rather than streaming it, because a pipeline's exit status in POSIX sh reports the last command rather than make. The capture file is allocated once and removed by the EXIT trap that already owns the failure list, and the run is pinned to LC_ALL=C: all three checks match English wording, one on make's and two on helm-unittest's, so a localized make would disarm the first of them silently, which is the failure mode this guard exists to remove. The locale reaches every package's recipe, not only make. Extend hack/helm-unit-tests.bats with eight tests. Three pin the new refusals; two pin what they must not refuse, a phony target with a colliding path and a sub-make reporting an unrelated target up to date; one pins that a chart which does run suites still passes; two pin behaviour that was already there, namely that a failing package is still reported by name and a package with no `test` rule is skipped rather than failed. That last one carries a second package that does run a suite, because with only the target-less package present the script takes its early exit above the failure summary and the assertion holds whatever the script did. The fixtures stub make output instead of invoking helm, so they need no plugin installed. Both tests that assert a package is named in the failure summary match the summary's " - " prefix rather than the bare path. The script announces "Running tests in $dir" before it runs anything, so the bare path is present in the output whatever the summary does; matching it asserted nothing, and the summary could stop naming packages entirely with the suite still green. The failing-package fixture reports a passing suite count alongside its non-zero exit, so that the exit status is the only thing that can decide the outcome. A fixture that fails without printing the marker is refused by the positive-evidence check first, which leaves the exit-status branch unpinned. That shape occurs in the tree: a `test` target that runs helm-unittest and then a second step, a render-parity script or a vendored-version guard or go tests, emits the marker and still fails. The two tests already in that file cover a package reached under packages/tests, and their fixture printed a marker and nothing else. That is the shape the positive-evidence check refuses, so the fixture now also emits the line a real run emits; without it both would fail for a reason unrelated to what they assert. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
b8a5b9ba1d |
test(e2e): capture worker CPU throttling counters on node-join failure
When a tenant worker misses the node-Ready deadline, the sandbox node it runs on is routinely at half its capacity with a clean kernel log. In that state "the host starved the VM" and "the VM was held at its own CFS ceiling" produce identical evidence: node-level utilisation reports what the node used, never what the container was allowed. The CFS counters are the only place the difference is recorded, and nothing in the tree collected them for the tenant workers, so every diagnosis of this failure so far had to be inferred from outside. Read them from the kubelet's cAdvisor endpoint on the node hosting each worker. Five series carry the whole answer and cAdvisor publishes all of them per container, so nothing is computed here: the scheduled and throttled period counts, the time spent throttled, and the quota and period that form the ceiling. The throttled counters alone say a container hit some ceiling, not which one, and a VM capped at one core and a VM capped at eight are the same number without the quota. Absence of the quota series is itself a reading rather than a gap. cAdvisor emits it only when the quota is non-zero, so a container reporting the period and no quota is one running uncapped - which is one of the two answers this collector exists to distinguish, arriving without being derived. Reading the kubelet rather than the container matters beyond convenience. A reader running inside the container shares the cgroup it measures, so it slows down in proportion to how much the answer matters and would need a budget sized for the pathological case; the kubelet is outside that cgroup. It also drops any dependency on what the container image ships and on how the host lays cgroups out. The namespace is not a worker filter and is not used as one: it also holds the tenant control plane, whose apiserver and etcd carry CPU limits of their own and can be genuinely throttled. The Pod name carries the second half of the filter, so a throttled apiserver cannot answer under a heading that says worker. The walk is over nodes rather than Pods, because the endpoint is per node and one read covers every worker it hosts. It is gated on the diagnostics phase budget, and placed first among the five gated collectors: the budget declines whatever has not started when it runs out, its own ceiling is four bounded reads rather than the minutes the four after it can spend, and it is the only one whose question has no other answer in the tree. That order is a trade, stated here because it is a decision rather than a detail - it puts the guest serial console one place further from the budget, and that capture is the only one describing a worker which never reached apid. Both reads take the block's own read-budget knob rather than a literal equal to it, so lowering that knob lowers these too. Each keeps its own exit status rather than routing through the shared helper, which always returns zero by design: this collector has to tell "the kubelet was not read" from "the kubelet answered and carried no worker", and only the status separates them. The stream is captured whole and filtered after, since a pipeline reports its last command's status and would otherwise hand that decision to grep. Every way a read can come up short is named rather than left as zero bytes: a kubelet that was not read, one that answered with nothing for the namespace, a stream cut short, and exit 124 - which cannot distinguish this collector timing out from the read exiting 124 on its own - each get their own line, and each says whether an error log exists to explain it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
c681ae1243 |
test(tenant): anchor the operator-gate absence and refresh a spent reason
The two "carries no operator gate" tests asserted only `notExists: spec.dependsOn`. A bare absence assert passes on a template that renders no document, so toggling `seaweedfs` and `gateway` off left both green while the releases they guard were gone. Measured: all 5 tests passed with the templates rendering nothing, and 2 of them fail once the name is pinned alongside the absence. An earlier check on these two asserts injected a `dependsOn` and watched them redden, which proves only that they catch an added gate. That is the other direction; the one that matters for a negative assert is the document disappearing. Both suite headers also justified their `templates:` list with a reason that has expired: keycloakgroups.yaml now guards its `.Values._cluster` index, so a no-values render of the whole chart succeeds. The list is still load-bearing, for the reason the sibling suite already records. It keeps an unrelated template out of this suite's result: with a nil dereference added outside the guard of a template neither suite asserts on, both suites stay green with their list and error without it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
5d436fad43 |
fix(build): declare the root test target phony
The root Makefile declares nine of its targets phony and `test` is not among them, while a `test/` directory sits beside it. Make therefore takes that directory for the target's product: `make test` prints "'test' is up to date", exits 0, and the recipe -- which installs the platform and runs the e2e suite -- never executes. docs/agents/overview.md advertises the command as the way to run the full suite, so someone following it by hand gets a green exit and no tests. CI is unaffected: the workflow calls test-controllers, and the e2e path is driven out of packages/core/testing directly. Every other undeclared target in this file is unshadowed today, so this is the only one that misbehaves, and one name closes it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
91b9f58057 |
fix(hack): declare the shared package targets phony
package.mk set .PHONY with `=` rather than `:`, which assigns a variable named .PHONY instead of declaring anything phony, so none of the targets package.mk defines was phony in any package that includes it. Declare exactly the concrete targets package.mk defines: help, show, diff, apply, delete, suspend, resume, check and clean. The earlier list named update and image, which package.mk does not define — naming an undefined target in .PHONY creates it as an empty target, so `make image` and `make update` would report success without running anything in the many packages that lack those recipes. It also omitted check and clean, the targets a same-named file or directory could actually shadow. The neighbouring .DEFAULT_GOAL is a variable and correctly keeps its equals sign. A comment now records which of the two adjacent lines is which, so the corrected one does not pull the other into its shape by analogy. The trigger map in docs/agents/contributing.md called the two files a byte-for-byte copy, which was exact rather than loose: they were the same object. They stop being one here, so the entry now records that they were identical and no longer are, and asks for a diff-and-port, the wording the map already uses for the other copy that has diverged. It deliberately does not enumerate which lines differ: that set moves as the copy catches up, and an entry that names it would be wrong again by the next port. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
62204d4903 |
test(cozystack-controller): guard the Secret grant by effective permission
The suite's "does not grant a blanket wildcard verb on secrets" assert was a notContains over a rule object. helm-unittest compares every field it is handed by value equality, and `any: true` changes which fields are compared rather than how, so no spelling of that assert bounds what a verbs list may hold: `verbs: ["get", "*"]` satisfies all of them. Measured against the rendered ClusterRole, seven realistic spellings of a widened Secret grant left that assert green: a wildcard in resources, a wildcard in apiGroups, a "*" appended to the declared verb list, an extra field beside the verbs, deletecollection spelled out, and two where the grant is added as a separate rule. Those last two leave the whole suite green - appending apiGroups:['*'] resources:['*'] verbs:['*'] keeps the positive pin satisfied while the controller gains every verb on every Secret cluster-wide. Replace it with a bats guard over the rendered manifests. It unions the verbs that every ClusterRole and Role rule confers on core Secrets, expanding "*" on each of the three axes, and compares that union with the declared set. Equality rather than an upper bound: the controller is cluster-scoped, so a widening reaches every tenant namespace, and a narrowing breaks the two reconcilers that share the grant. The positive pin stays in tests/rbac_test.yaml. Value equality is the right primitive for pinning a rule as written and the wrong one for bounding what the rule may become. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
ea78d09dd0 |
feat(ci): auto-apply triage labels to issues
Issues created via the API or gh bypass the issue templates and arrive without any triage/* label; 252 of the 396 open issues carry none. Label issues on arrival (opened/reopened) and run a daily sweep as a backstop: issues already prioritised, assigned, or marked as epics get triage/accepted, the rest triage/needs-triage. A signal has to be on stale.yaml's exempt-issue-labels, because triage/accepted is itself exempt and a signal that is not would grant a permanent stale reprieve as a side effect of being labeled. That is necessary and not sufficient: lifecycle/frozen is exempt and still not a signal, because a lifecycle/* label says what the stale bot may do with an issue and nothing about whether anybody reviewed it. The sweep paces its writes and caps how many it makes per run, so a pass over a large backlog stays inside GitHub's secondary rate limits instead of failing partway through. Each run writes a job summary with the counts, the issue numbers per label, and both remainders it can leave behind, because a backlog pass logs a line per issue and nobody reads 252 of those to find out what happened. The manual dispatch defaults to dry-run, so the one entry point a human can reach writes nothing until somebody unchecks the box. The schedule is the one event that writes without being asked. Deleting a triage label does not undo a decision: an issue left with none is relabeled by the next sweep, so AGENTS.md's labeling section says to change a decision by replacing the label instead. No PR lane can exercise this workflow, so the first real run is a sweep over every open issue. A contract test pins the executable lines that bound that: the pull-request skip, the already-triaged skip, the pacing and the cap, the dry-run default, the pinned action, the token scopes, that no signal comes from the lifecycle/* namespace, and that the summary is written with the await that makes it flush. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
myasnikovdaniil
|
e944619e98 |
docs(release): add changelog for v1.6.1 (#3551)
This PR adds the changelog for release `v1.6.1`.
✅ Changelog has been automatically generated in
`docs/changelogs/v1.6.1.md`.
|
||
|
myasnikovdaniil
|
516be4abfe |
fix(cozystack-basics): normalize tenant hostname VAP comparisons with lowerAscii (#3208)
## What this PR does The Ingress, Gateway, and Route hostname `ValidatingAdmissionPolicy` objects compare tenant hostnames against the tenant apex with case-sensitive CEL. The resource-side host is always lowercase — the apiserver's hostname validation permits nothing else — but both apex operands can carry uppercase: the `namespace.cozystack.io/host` label value, which Kubernetes permits to carry uppercase and which the tenant chart writes verbatim from `tenant.spec.host`, and the templated `_cluster.root-host`, a free-form platform value that nothing validates for case. A mixed-case apex label wrongly denies a tenant its own legitimate lowercase in-apex hostname. An uppercase `_cluster.root-host` is the dangerous direction: the Ingress outside-root exclusion compares a lowercase host against the uppercase literal, never matches, so the exclusion stops excluding and the policy admits any non-wildcard host — every non-wildcard platform-apex name a tenant asks for, including a hostname belonging to another tenant and the root apex itself. Only the anti-wildcard gate still holds. This normalizes both operands of every hostname comparison with `.lowerAscii()` (host, apex label, and the templated root-host literal) across all three policies, so the match follows DNS case semantics. There is no behavior change in the default all-lowercase configuration. The Gateway policy's fail-open on an absent or empty host label is deliberately left as it was: that is the platform's one documented fail-open exception in `docs/security/threat-model.md`, so changing the posture is a security-model decision rather than a side effect of normalizing case. Its current shape is pinned by a test. Test coverage: - helm-unittest pins each comparison as a whole operand pair — equality and `endsWith`, across the rule-host and tls-host branches — so dropping the normalization from any single operand is caught. A pattern matching a lone `.lowerAscii()` would not: the apex appears four times in the Ingress expression, so stripping one operand still leaves three matches. - A rendering case with `_cluster.root-host: Example.ORG` pins the outside-root escape shut; it is red pre-fix. - e2e probes set a mixed-case apex on an isolated throwaway namespace and assert both halves of the match: a lowercase in-apex HTTPRoute and Ingress are still admitted (red pre-fix, green post-fix), while a foreign apex and a hostname ending with the apex without a dot boundary are still denied. Vacuous-pass guards re-read the persisted label and assert the probe apex really is mixed-case. Normalizing the apex at the write site — the tenant namespace template and `_cluster.root-host` — is tracked separately in #3328. It would not remove the need for this change: the host label is settable out-of-band by a trusted caller, so these policies must not assume the apex arrives normalized. Closes #3205. ### Screenshots N/A (no UI changes). ### Release note ```release-note fix(cozystack-basics): tenant hostname admission policies now compare against the tenant apex and the platform root apex case-insensitively. This closes a bypass where an uppercase `_cluster.root-host` made the Ingress policy's outside-root exclusion stop matching, so the policy admitted any non-wildcard host — including a hostname under the platform apex belonging to another tenant, and the root apex itself. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Hostname validation now consistently matches case-insensitively across Gateway, Ingress, HTTPRoute, and TLSRoute. * Mixed-case tenant and apex hostnames are accepted when they meet the intended domain boundaries. * Foreign domains and prefix-adjacent hostnames remain correctly rejected; validation behaves as documented when the tenant host is empty. * **Tests** * Expanded unit and e2e coverage for case-insensitive hostname rendering and boundary rejections across the supported resources. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
b496fb1ae8 |
docs(backups): note to-copy MongoDB restore needs target creds that can read the source bucket
A to-copy restore reads the dump from the source backup's bucket/endpoint but authenticates with the target cluster's own S3 credentials (the source release's Secret dies with it in a DR scenario). Document that the target's credentials must therefore be able to read the source bucket - the common same-bucket cozy-backups case - and that a cross-account/bucket target fails loudly with AccessDenied rather than silently restoring nothing. Addresses the MINOR credential-scoping note from PR review. Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
b30c7f4272 |
fix(backups): requeue MongoDB restore on a transient source-CR read error
resolveMongoDBBackupSource folded two failure classes into one error return:
a transient API read error on the live PerconaServerMongoDBBackup CR (cache
not yet synced, apiserver hiccup) and the terminal no-restorable-destination
case. The RestoreJob caller turned every error into markRestoreJobFailed, so a
retryable read error failed a restore that would have succeeded on the next
reconcile. The backup path already treats the symmetric live-CR read error as
transient (return ctrl.Result{}, err).
Wrap the transient read error in a new errTransientRestoreSource sentinel and
requeue on it at the caller; only a genuinely unresolvable source stays
terminal. Adds a non-vacuous interceptor-driven test.
Addresses the MINOR robustness note from PR review.
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
|
||
|
Andrey Kolkov
|
b97c443a16 |
fix(backups): harden MongoDB restore auth + PITR option handling from review
Address the two blocking findings in PR #3562: - Restore into a differently-named target now authenticates with the TARGET cluster's own S3 credentials instead of the source's. The backupSource carried the source release's -s3-creds Secret, which is deleted together with the source app — the disaster-recovery case backups exist for — leaving a to-copy restore unable to reach S3. The restore replays into the target (gated on backup.enabled just above), whose own -s3-creds Secret is present and grants access to the same bucket; only the credential reference is swapped, bucket/prefix/endpoint still come from the source archive. Falls back to the source reference when the target declares no discoverable s3 credentials (in-place restore resolves to the same Secret, so it is a no-op there). - A non-string recoveryTime (spec.options is free-form runtime.RawExtension, so {"recoveryTime": 20260805} passes admission) no longer silently degrades a PITR request to a plain snapshot restore reported Succeeded. When the typed decode fails and the blob carries a recoveryTime key, the RestoreJob now fails terminally — the same guarantee pitrSpec already makes for a malformed RFC3339 string. Minor findings also addressed: - BackupJob tolerates a not-yet-present MongoDB app CR with the same bounded grace the psmdb-CR existence gate uses, instead of failing terminally on a GitOps single-commit ordering race. - examples: the to-copy target writes into its OWN S3 prefix (mongodb-target/) rather than the source's, so the two clusters' oplog and backup metadata no longer interleave under shared keys (both run replset rs0). - examples: wait_psmdb_ready no longer treats a transient state=error as terminal — the cluster state is recomputed each reconcile and can blip through error during replset init before converging to ready. Unit tests cover the target-credential selector (preferred/default/sole storage) and the load-bearing recoveryTime-key detection. Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
0d4a3e3134 |
refactor(backups): make MongoDB PITR option uniform with CNPG + warn on unknown restore options
PITR: replace the bespoke spec.options.pitr{type,date} blob with a single
spec.options.recoveryTime RFC3339 string, matching the CNPG strategy's option
exactly (CNPGRestoreOptions.RecoveryTime). The driver converts recoveryTime to
the psmdb Restore's pitr{type:date,date} in the operator's "YYYY-MM-DD HH:MM:SS"
UTC format. An empty recoveryTime restores the snapshot as-taken (documented
difference from CNPG, whose empty value means latest WAL — a psmdb logical
backup is already consistent). A non-RFC3339 recoveryTime fails terminally.
Unknown options: parseMongoDBRestoreOptions now also returns the set of
spec.options keys the driver does not recognise; reconcileMongoDBRestore emits a
UnknownRestoreOption Warning event per unknown key (and logs it) instead of
silently ignoring it — so a "recoverytime" typo no longer silently degrades a
PITR restore to a snapshot restore. (BackupJob carries no spec.options blob, so
there is no backup-path equivalent; the only backup-time input is the free-form
BackupClass parameters used as template vars.)
Docs + examples updated to recoveryTime; tests cover the RFC3339→UTC mapping,
empty/invalid recoveryTime, and unknown-key reporting.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
|
||
|
Andrey KolkovandClaude Opus 4.8
|
540bb94f58 |
test(backups): cover createMongoDBBackupArtifact producer seam
Address branch-review blocker: add TestCreateMongoDBBackupArtifact_ArtifactShape, mirroring the MariaDB sibling. createMongoDBBackupArtifact writes the driver-metadata keys (backup-name/namespace/destination), the Status.Artifact URI, and the persisted snapshot that the restore path reads back — the producer half of a seam previously exercised only end-to-end. A key-name drift between this producer and resolveMongoDBBackupSource would otherwise pass every unit test and surface only in e2e. The test asserts the metadata keys, phase, artifact URI, and that the snapshot decodes to a usable by-reference backupSource; a second case pins the empty-destination path (nil Artifact, no destination key). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
bc5e47c64f |
fix(backups): add explicit MongoDB cleanup-on-delete branch + its guard test
Address branch-review recommendation: give the MongoDB strategy an explicit case in BackupReconciler.cleanupOnDelete, matching CNPG / Altinity / MariaDB / FoundationDB. A deleted MongoDB Backup artifact previously fell through to the Velero default (a safe no-op — MongoDB driverMetadata never carries velero.io/backup-name), but the explicit branch documents the "we do not own the psmdb S3 archive" contract and guards the seam against a future shared helper incidentally stamping velero.io/backup-name onto MongoDB metadata. Ships with TestBackupCleanup_MongoDB_DoesNotFallThroughToVelero, mirroring the one-test-per-branch convention its three siblings follow: it seeds the velero metadata keys + a matching velero.io/Backup and asserts no DeleteBackupRequest is synthesised and the Velero Backup survives — so deleting the branch makes the test fail, which is what makes the guard self-verifying. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey KolkovandClaude Opus 4.8
|
dff56aa998 |
feat(backups): add a native MongoDB (psmdb) backup-strategy driver
Adds a MongoDB (Percona Server for MongoDB / psmdb) backup-strategy driver so `apps.cozystack.io/MongoDB` reaches feature parity with the CNPG, MariaDB and ClickHouse native drivers — logical backups, restore-to-differently-named instances, and point-in-time recovery through the unified BackupJob/RestoreJob/BackupClass/Plan interface (closes #3545). The driver orchestrates the operator's PerconaServerMongoDBBackup / PerconaServerMongoDBRestore CRDs: - BackupJob: creates a one-shot PerconaServerMongoDBBackup, polls status.state, and persists the S3 destination + storage descriptor (by-reference, no credentials) onto the Cozystack Backup artifact. - RestoreJob: drives PerconaServerMongoDBRestore via backupSource (read back from the source backup status, snapshot fallback) so the same artifact restores in-place or into a differently-named instance; PITR via spec.options.pitr (date/latest). Because psmdb resolves the backup storage by name against the cluster's spec.backup.storages, the driver names the chart's storage (s3-storage) via the cozy-default-mongodb Strategy rather than injecting it, and surfaces a clear Ready=False precondition when a target has backups disabled. Wiring: - api/backups/strategy/v1alpha1: MongoDB strategy type + generated CRD/deepcopy. - internal/backupcontroller: psmdbstrategy_controller + psmdbtypes/mongodbapp partial-schema mirrors; dispatch, supported-kinds and scheme registration. - backupstrategy-controller: cozy-default MongoDB route, cozy-default-mongodb Strategy CR, and psmdb.percona.com RBAC. - apps/mongodb: backup.insecureSkipTLSVerify knob (psmdb s3 storage has no CA-bundle field) so backups can target Cozystack's self-signed seaweedfs. Tests + e2e: - unit tests for the driver (validation, ensure-idempotency, precondition, backupSource resolution, snapshot round-trip, PITR option parsing). - examples/backups/mongodb + a mongodb-2-backup-roundtrip Chainsaw suite that drives run-all.sh end-to-end (sentinel document round-trips through S3 into a restored copy while the source stays untouched), per docs/agents/e2e-testing.md. - docs/operations/backup-classes.md updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
scooby87
|
a242f34593 |
fix(clickhouse): assign DB_LIST so backup retention cleanup runs (#3760)
## What this PR does The legacy per-instance ClickHouse backup CronJob (rendered when `backup.enabled=true`, `backup.schedule` is set and `backup.useSystemBucket=false`) never applied its retention policy. `packages/apps/clickhouse/templates/backup-script.yaml` printed `echo DB_LIST=$(...)` instead of assigning `DB_LIST`, so the variable stayed unset and the cleanup loop `for db in $DB_LIST` ran zero iterations — `restic forget` and `restic prune` were never invoked, snapshots accumulated in object storage indefinitely despite the configured `cleanupStrategy`, and the Job still exited 0 so nothing surfaced the failure. This drops the stray `echo` so `DB_LIST` is assigned from the table list and the retention cleanup runs as configured. MariaDB's equivalent backup script already assigns `DB_LIST` correctly; this brings ClickHouse in line. A helm-unittest regression over the rendered `backup.sh` asserts the assignment is present and the `echo DB_LIST=` typo is gone. Verified with `make -C packages/apps/clickhouse test` (18/18 pass). Fixes #3758. ### Screenshots <!-- No UI changes. --> ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. The change is a one-line shell fix inside a Helm template plus a helm-unittest case — it touches no `values.yaml`, `values.schema.json`, `README.md`, `ApplicationDefinition`, version enum, variant/bundle, release asset, `hack/` layout or namespace. No downstream repository restates any of this. - [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(clickhouse): ClickHouse backup retention now prunes old snapshots. The scheduled backup job's cleanup step never ran because `DB_LIST` was left unset, so snapshots accumulated in object storage indefinitely; it now honors the configured retention (`cleanupStrategy` / `--keep-last=N`). ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed backup processing so table lists are assigned correctly, allowing retention cleanup to run as expected. * Removed unintended output of the shuffled table list during backups. * **Tests** * Added regression coverage to verify correct backup list assignment and prevent the previous malformed assignment from recurring. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Alexey Artamonov
|
2b5c85cde7 |
fix(clickhouse): assign DB_LIST so backup retention cleanup runs
The legacy per-instance backup script printed `echo DB_LIST=$(...)` instead of assigning DB_LIST, leaving it unset. The cleanup loop `for db in $DB_LIST` then ran zero iterations, so `restic forget` and `restic prune` never executed and snapshots accumulated in object storage indefinitely despite the configured cleanupStrategy. The Job still reported success, hiding the failure. Assign DB_LIST from the shuffled table list so the retention cleanup loop runs. Add a helm-unittest regression asserting the rendered backup.sh assigns DB_LIST and no longer contains the `echo DB_LIST=` typo. Fixes #3758 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
myasnikovdaniil
|
d04231829a |
docs(hack): fix a wrong claim about grep-error propagation in two contract tests (#3620)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does `hack/promote-gate-contract.bats` and `hack/release-freeze-contract.bats` both carried the same wrong comment above their `code_lines` helper: it said a real grep error (exit 2) fails a pin through the helper's own exit status, distinct from an empty match (exit 1). That was never actually the mechanism. `code_lines` is always piped into a further `grep` in both files, never the last stage of a pipeline, so ordinary (no `pipefail`) pipe semantics discard whatever it returns on its own, regardless of runner. `hack/cozytest.sh`, the runner these tests actually execute under in CI, also appends a `return 0` to every line that is exactly `}`, `code_lines`' closing brace included, making that exit status even less trustworthy as a signal. What catches a real grep error is the output, and only for one class of pin. The error empties `code_lines`' stream, so a pin that requires something to be present in that stream fails on it. A pin demanding an absence stays blind: an exact count of zero is what an emptied stream produces anyway, and what the pin was written to assert. Both files hold pins of that shape. Nor is the `[ -n "$block" ]` guard at the top of each test the backstop: a block here is a bare awk extraction, upstream of `code_lines`. The new comment says both things and stops there. Comment-only: `code_lines`'s body is untouched in both files, and all tests pass under both `bats` and `hack/cozytest.sh` before and after. Known limitation of the wording: the comment says the top-of-test `[ -n "$block" ]` guard cannot see a filter failure, which is true, but both files also carry non-empty guards on values that did pass through `code_lines` (`promote-gate-contract.bats:72,73,165` and `release-freeze-contract.bats:166,236,327` via `step_line`), and those do fail on an emptied stream. The discriminator is whether the guarded value came through the filter, not where the guard sits in the test. ### Screenshots Not applicable, no UI change. ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified comments describing how code-line filtering errors are detected. * Documented pipeline status behavior and why output-based assertions are used instead of relying solely on command status codes. * No user-facing behavior or executable logic changed. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
cf6bf2e379 |
fix(ci): stop labeled events from cancelling and re-running backports (#3569)
## What this PR does A run joins its concurrency group before any job-level `if` is evaluated, so `prepare`'s guard in `backport.yaml` is too late to help. By the time it works out that a label event is irrelevant, that run has already cancelled whatever was in flight for the same PR. Labels set with the default `GITHUB_TOKEN` start no run, which is why the in-repo labelers never showed this. Third-party GitHub Apps do start runs, and they arrive in bursts. The delivery survived, which is why this went unnoticed for so long. The old guard admitted a run on the PR's cumulative label set, so the last label of a burst, the one nothing cancels, requalified and redid the work. In the bursts on record the kill lands within seconds of the merge run starting, well before the cherry-pick, so the cost was a restart rather than a lost backport, recorded as nothing more than a cancelled run sitting next to a green one. Two changes to the concurrency key, and both are needed. A label event that requests no backport now goes into a group of its own, so it can neither cancel nor displace the run doing the work. A label event that does request one queues behind that run instead of killing it, because `backport` and `backport-previous` are separate requests and both have to finish. Splitting alone would let `backport` cancel the merge-triggered run. Queuing alone would let an unrelated label evict a pending request, since the group holds a single pending run and a newer one replaces it. `prepare`'s condition is scoped in the same commit. It read the PR's cumulative label set, so once a merged PR carried `backport`, any later unrelated label re-entered the job to redo a backport already delivered. That run used to get cancelled by the next label in the burst. After the change above it runs to completion, so the scoping has to land here rather than as a follow-up. The coupling runs the other way too, and it is the sharper half. That redundant run is exactly what redelivered the backport the burst had just killed, so narrowing the guard on its own would convert a noisy delivery into a missing one. Neither change is safe to land without the other. Both jobs also gain a `timeout-minutes` ceiling, which is a consequence of the queuing rather than housekeeping. A cancelling key disposed of a stuck run by killing it; queuing makes the next genuine request wait behind it instead, on the six-hour job default, which turns one wedged job into a six-hour hole in the release line. That ceiling reaches a stuck job and nothing else: a run can also wedge at the run level with every job already finished, and `timeout-minutes` has no run-level equivalent, so nothing in the workflow reaches that state. `docs/release.md` now tells an operator to look for a run still in flight and clear it before re-applying the label, because the retry queues behind it rather than replacing it. The ceiling bounds execution, not the wait for a runner, and thirty minutes is generous rather than calculated. Execution on run 29844315422 was six seconds for `prepare` and two and sixteen for the matrix legs, against 9m24s, 5m28s and 5m45s of runner queue for the same three jobs. That queue is a third unbounded case: GitHub's limits page says "job execution time" for the six-hour job cap while spelling out that the 35-day run cap "includes execution duration, and time spent on waiting and approval", and [orgs/community#50926](https://github.com/orgs/community/discussions/50926) shows a job sitting six hours on "Waiting for a runner to pick up this job" under `timeout-minutes: 5`. So the value is picked to leave room for a cherry-pick far larger than any on record while replacing the six-hour default, and nothing here is sized against the queue, because nothing here can be. This removes a mitigation, and the trade is worth naming on both sides. Under an unconditional cancel a run wedged at the run level cleared itself, because the next event killed whatever was sitting in the group. That is the same behaviour that killed live backports, so the mitigation is inseparable from the bug being fixed: keeping it means keeping label events that cancel a backport in flight. What it buys is a change of failure mode rather than one fewer failure. A backport lost silently, discovered weeks later as a fix missing from a release line, becomes a delay the operator can see in the run list, with the recovery written into `docs/release.md`. There is no lossless alternative here: `queue: max` removes the eviction but only by dropping the conditional cancel entirely, and the group key cannot tell a manual retry from a second genuine request. The run cited above, 31232645133, is still live and retained on purpose: it is the only observed instance of the run-level wedge, so clearing it would remove the evidence for the paragraph that describes it. Its exposure is one pull request, #3262, which carries no backport label, so nothing is queued behind it. The old condition also carried a disjunct that could never decide anything. A `labeled` payload already lists the new label in `pull_request.labels`, so `contains` was true whenever the explicit `github.event.label.name` check was, and the name check only ever agreed with a `contains` that had already matched. The restructure drops it by construction rather than by deletion: the name check is now the whole of what a `labeled` event may match on. ### Why this no longer touches `pull-requests.yaml` `main` fixed it there by splitting the group key, then refined that so a `full-e2e` label stays in the main group instead of publishing a second `E2E Tests` status on one SHA. That is the better mechanism, it neither queues nor delays, and `gate-concurrency-contract.bats` now pins `cancel-in-progress: true` in that file, so what this PR originally did would turn it red. Nothing was left to add there, so the hunk is dropped and its pin with it. The split-key idea is applied in `backport.yaml` instead, where it did not exist. One claim from the earlier description is withdrawn. It said the leftover risk was a duplicate backport PR, opened when the original had merged and its branch had been deleted before the queued run pushed. `main` has since gained a guard that lists closed PRs on the `backport-<n>-to-<target>` branch and skips the target when one of them merged. Branch deletion does not defeat it: `GET /branches/backport-3510-to-release-1.6` is 404 while the same head filter on `/pulls?state=closed` still returns the merged PR. So a redundant run only cost runner minutes, and the scoping above removes the run. ### Screenshots Not applicable, no UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff. The only workflow it cites is `tags.yaml`, for the release-prep behaviour `cozystack/ccp` documents. Every `hack/` trigger it lists either names a specific file another repository mirrors or gates on, or covers moving and renaming under `hack/`. This PR appends tests to a contract file the map does not name, and moves nothing. No package, chart, values file, CRD, image reference, namespace, variant or node prerequisite in the diff. - [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(ci): stop `labeled` pull request events from cancelling the automatic backport run already in flight. A burst of labels from a third-party app could previously kill the backport the merge had just started, leaving the work to be redone by whichever label arrived last. ``` |
||
|
Aleksei Sviridkin
|
4ac02f59d5 |
fix(e2e): stop the sandbox cluster CIDR overlapping the pod range (#3750)
## What this PR does The sandbox Talos config and the platform install step both used `10.244.0.0/16`, one as the Kubernetes cluster CIDR and the other as the kube-ovn `POD_CIDR`, so two allocators drew from one range. kube-ovn owns pod IPAM and allocates flat from `POD_CIDR`. Cilium is chained behind it (`generic-veth`, target `kube-ovn`) and allocates no pod addresses at all, but it still carves its own per-node router IP and Ingress IP out of `Node.spec.podCIDR`, which kube-controller-manager slices from the cluster CIDR. Sooner or later kube-ovn hands a pod an address cilium already holds. When that address is the Ingress IP the agent refuses the sandbox with `IP ipv4:<addr> is already in use`, and cilium never relents on it, so the pod proceeds only if something reschedules it onto a different address; in the run this was filed from, the same address was refused seven times across six pod UIDs before the suite ran out of budget. When it is the router IP nothing complains and only host-to-pod traffic breaks. This moves the sandbox cluster CIDR to `100.65.0.0/16`, which no other range in the tree uses, and adds a guard so the two cannot be brought back together by accident. Pod addresses do not change. They still come from kube-ovn out of `10.244.0.0/16`, and cilium does not require a chained endpoint's address to sit inside its own allocation CIDR, so only cilium's two per-node infrastructure addresses move. The quiet variant gets fixed twice over, which is worth spelling out because only half of it was obvious. Under `ipam.mode: kubernetes` cilium also installs a route for the node's own pod CIDR via `cilium_host`. On the current default that route covers `10.244.a.0/24` on a node whose pods actually live behind `ovn0`, so it shadows real pod addresses. Moving the cluster CIDR both stops the router IP from ever being handed to a pod and retargets that route at `100.65.a.0/24`, where it shadows nothing. Excluding the addresses on the kube-ovn side instead is not an option: cilium picks its offsets by random scan over the node's range, so there is no static set to exclude. The collision shows up in two shapes, and both have been seen. Loudly, on the Ingress IP, where the agent names the address in the event, which is the failure https://github.com/cozystack/cozystack/issues/3736 was filed for. Quietly, on the router IP, where the pod comes up and the only symptom is `connection refused` on a kubelet probe, with `ip route get <podIP>` on the node resolving to `local ... dev lo`; that variant is documented in https://github.com/cozystack/cozystack/issues/3445 and https://github.com/cozystack/cozystack/issues/3615. They were being tracked as unrelated flakes in different suites before the shared cause was identified. ## What the guard covers, and what it does not `hack/sandbox-cidr-disjoint.bats` pins the configuration, not the behaviour. It reads the values out of the tree and fails when they overlap. It does not reproduce the collision, which needs a running cluster. It checks the invariant rather than the literals, so either value may move as long as they stay disjoint, and it checks the cluster CIDR against the service and join ranges too, because landing on one of those instead is the next way to get this wrong. An extraction that comes up empty, duplicated or misshapen fails the test instead of shrinking the comparison, and comments and blank lines inside the list are skipped rather than read as the end of it. It is not a YAML parser, though: anything it does not read as a bare dotted quad fails the shape validation rather than being understood, which is the safe direction but not the same promise. Those three are the allocator ranges and nothing more. The sandbox node network, the load-balancer pool and the transit range of a component that is off by default are not read, so the guard is not a complete map of what the cluster CIDR has to avoid. An IPv6 entry fails the shape validation rather than getting an overlap check, so dual-stack means extending the guard rather than passing through it. ## Residual risk I did not prove from source that cilium is the only consumer of `Node.spec.podCIDR` in this topology. What I did check is that kube-proxy is disabled and that `local-ccm` runs the node-lifecycle controller only, with no route controller. This diff selects the full e2e suite, 21 Chainsaw suites with `gateway` and `mongodb` among them, so a consumer I missed surfaces as a red run on this PR rather than staying an assumption. Split of what covers what: the static checks and the unit guard cover the configuration, and the e2e run covers the behaviour. Nothing short of a green suite answers whether cilium's router and Ingress addresses behave once they sit outside the range kube-ovn allocates from. ## A check that is not "the suite went green" `hack/e2e-cilium-endpoint-leak-healer.sh` already prints a line of its own for this exact collision, so there is a signal to read that does not depend on the overall verdict. In the `cozyreport` artifact it lands at `snapshots/<suite>/host/namespaces/kube-system/v1/pod/cilium-leak-healer-*/healer/current.log`, and the lines to grep for are `IP is this node's Cilium ingress IP`. In the run this was filed from there are 42 of them, all naming the same address. After this change that line cannot be produced by an address inside the kube-ovn range, because there is no longer an address that is in both. So the expectation is zero from the first run, not zero on average: one occurrence contradicts the change rather than being noise, and is worth stopping on. A green suite can be green for many reasons; this line disappearing has only one. ## Scope This fixes the sandbox and nothing else. For a real installation the cluster CIDR lives in the operator's own machine configuration, outside this repository, so the same collision is still reachable there. https://github.com/cozystack/cozystack/issues/3740 tracks the product side, including the finding that the install documentation does not merely allow the overlap but instructs operators to create it, by requiring `podCIDR` to match the cluster's pod CIDR. There is a tradeoff in this worth naming rather than leaving to be discovered. Talos defaults `podSubnets` to `10.244.0.0/16` (`DefaultIPv4PodNet`, `pkg/machinery/constants/constants.go:888` at v1.13.6, the version this tree ships), and the platform defaults `networking.podCIDR` to the same value, so an operator who takes both defaults gets the overlap. Until now the sandbox shared that topology and was the one place the bug surfaced where anyone would see it. After this change it no longer reproduces the operator-facing form, and nothing else does either. That is the right call for CI, which should not be wedged by a defect it did not introduce, but it does mean the production exposure loses its only accidental detector. Closes https://github.com/cozystack/cozystack/issues/3736 To be explicit about what closing that issue does and does not mean: this removes the reproduction in the sandbox and leaves the product configuration untouched. The collision is still reachable for users, and https://github.com/cozystack/cozystack/issues/3740 is where that lives. The issue text describes the root mechanism, which is wider than its title, so the closure covers the e2e failure it was filed for and not the platform default. ### Screenshots Not a UI change. ### Downstream repositories No box is ticked deliberately, and no follow-up PR is opened, so a human decides. Walking the trigger map against this diff: both `hack/e2e-prepare-cluster.bats` entries in it are scoped to node prerequisites, the kernel modules a node must load, the containerd settings, the LVM `global_filter` and the cluster domain. This diff changes none of those, it changes a network range, so no entry fires literally. The underlying constraint does reach several of them all the same. `cozystack/talm` sets `podSubnets: [10.244.0.0/16]` in `charts/cozystack/values.yaml`, the same range the platform uses for `networking.podCIDR`, so the documented Talos bootstrap produces the overlap this PR removes from the sandbox. `cozystack/ansible-cozystack` reaches it from the other side: `roles/cozystack/defaults/main.yml` sets `cozystack_pod_cidr: "10.42.0.0/16"` and renders it into `networking.podCIDR`, while its inventories set `cluster-cidr: 10.42.0.0/16`, so those two are identical as well. And `cozystack/website` prescribes the equality outright, `install/kubernetes/generic.md` stating that `podCIDR` and `serviceCIDR` must match the Kubernetes cluster configuration. No issue is filed there yet on purpose: which side moves is not decided, and asking talm to change `podSubnets` would be the wrong request if the platform ends up moving instead. That decision belongs to https://github.com/cozystack/cozystack/issues/3740. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Updated cluster networking ranges to avoid conflicts with Kubernetes, kube-ovn, kilo, and Cilium networks. - Added validation to detect missing, invalid, or overlapping cluster and platform CIDR ranges. - Preserved safeguards for environments where network ranges overlap, with clearer diagnostic logging. - **Documentation** - Documented network separation requirements and clarified expected sandbox networking behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
e0d6c6be92 |
fix(ci): stop labeled events from cancelling and re-running backports
A run joins its concurrency group before any job-level `if` is evaluated, so `prepare`'s guard cannot stop a label event from disturbing the run already backporting a PR. Labels set with the default GITHUB_TOKEN start no run, but labels set by third-party GitHub Apps do, and they arrive in bursts, so each one cancelled the backport the merge had just started. The delivery survived anyway, which is why this went unnoticed: the old guard admitted a run on the PR's cumulative labels, so the last label of a burst requalified and redid the work. In the bursts on record the kill lands within seconds, well before the cherry-pick, so the cost was a restart rather than a lost backport. Move a label event that requests no backport into a group of its own, and let one that does request a backport queue rather than cancel. Both halves are needed. `backport` and `backport-previous` are separate requests that both have to complete, so they share a group and must not cancel each other, while every other label does no work here and only has to stay out of the way. Scope `prepare` to the labels each trigger is entitled to read at the same time. Reading the cumulative label set on a `labeled` event let any later unrelated label on a merged PR still carrying `backport` re-enter the job to redo a backport already delivered. That redo is also what redelivered a killed backport, so narrowing the guard alone would turn a noisy delivery into a missing one: neither change is safe to land without the other. Bound both jobs with `timeout-minutes` for the same reason, since a queued request now waits on the run ahead of it rather than replacing it. The ceiling bounds execution and not the wait for a runner, which no job-level key reaches, so 30 is generous rather than calculated: every execution measured on this workflow has been seconds, against minutes of queue that nothing here can bound. Document the recovery path the narrowing removes: with no accidental re-run left, re-applying whichever backport label the PR already carries is how an operator retries a merge-triggered run that died. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
ec6a835251 |
fix(manifestutil): report one message when the CRD wait is cancelled (#3742)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does `WaitForCRDsEstablished` reported a cancelled context two different ways. The check at the top of the poll loop returned a message with no CRD name, the `select` at the bottom named the CRD the poll had stopped on. When a tick and the deadline come ready in the same instant, `select` picks between the ready cases at random, so one cancellation reached the operator either way. Both callers wrap this as `CRDs not established after apply`, so the CRD name is what makes that log line actionable, and whether it is there was a coin toss. `TestWaitForCRDsEstablished_timeout` asserts on the name, which is where this surfaced: its 2s deadline is an exact multiple of the 500ms ticker, so the two channels come ready together on the fourth tick and the assertion failed at random. The message is now built in one place and returned from both exits, so the two are indistinguishable by construction rather than by inspection. The name is carried across iterations, because a cancellation noticed at the top of the loop has no poll of its own to take one from, and it starts as the first CRD of the set for a wait cancelled before any poll observed anything. That start value is a stand-in rather than an observation, and the comment on it says so; the alternative, an empty name and a second phrasing for it, would leave the loop-top message with nothing to assert against and no test able to catch its return to the unnamed form. Deleting the top check instead, so there is literally one exit, is the smaller diff and it moves the name to the head of the list in the very interleaving this PR is about. A client refuses `Get` on a dead context at the first name in the list whichever CRD is actually lagging, so a poll entered on a cancelled context renames the report to that first name. That premise is about the production client, `client.New` at `cmd/cozystack-operator/main.go:176`, uncached, so the request is rejected before it is issued, and it is worth saying that nothing in the test surface pins it: replacing the `ctx` passed to `Get` with `context.Background()` survives the whole package, and the fixtures that need a context-honouring client check `ctx.Err()` by hand. That line predates this change and is untouched by it. Measured with a client that honours the context: with the check, a wait that re-enters the loop after the deadline keeps the name its last poll settled on; without it, the report moves to the head of the list. The check closes that window and no more than that. A context that dies part-way through a poll still records the first `Get` to fail, exactly as merge base does, because this branch cannot tell a refused request from a CRD status. That is unchanged here and written up separately. Four tests cover the ways this wait can end on a cancelled context: before any poll has observed the set, cut off inside a poll, cancelled while waiting for the next tick, and re-entering the loop on a context that died during that wait. Two of them are red on merge base and are the regression pins for the defect above, both deterministically rather than by repetition: merge base answers a wait that starts cancelled, and a wait that re-enters the loop cancelled, with the unnamed message every time. Its loop-top exit is a `select` whose `Done` case is ready, and a ready case always beats `default`. The middle two are green on merge base, which gets those cases right; they are here because each catches a specific way of breaking the fix. Every one of the four is the sole killer of at least one mutation, which is the property worth having: it says none of them is redundant. They compare the whole message rather than searching it for a name, because a second exit that happens to carry a name is still the defect being fixed here, and a test that only looks for the name cannot see it. The re-entry test needs the bottom `select` to take the tick while cancellation is already pending, which against a real context is the same coin toss the bug rides on. The double it uses hands out an open `Done` for the single read that `select` makes in that iteration and a closed one for every read after, so the tick wins by construction and the next iteration sees a context cancelled through both `Done` and `Err`, the way a real one is. That pairing is guaranteed rather than incidental: `cancelCtx.Err` reads `<-c.Done()` before returning a non-nil error, so a double that reported cancellation through `Err` alone would be modelling a state no context can be in, and would pin the shape of the check rather than its behaviour. One property of that double is worth knowing before editing the loop: it hands out its open and closed channels per read, so a `Done()` read added inside the poll would consume the open one and leave the bottom `select` a closed channel, quietly turning this into a second bottom-exit test that still passes. The failure direction there is false-green, which is the worse half, and the cheap guard is a lower bound on elapsed time inside that test, a run that leaves through the loop top spends a ticker interval getting there, one that leaves at the bottom of the first iteration does not. Not done here, because the test is correct today and provably kills the mutation it exists for, so the hazard is conditional on a future edit. The timeout test does reproduce locally under load: 6 failures across 400 runs on the unfixed code while I was measuring, spread over five batches, the last of which saw none. #3719 reports that it does not reproduce locally, so this is worth writing down before someone spends an evening re-testing that. That spread is also why the number is an order of magnitude and not a rate. `TestInstall_crdNotEstablished` in `internal/crdinstall` has the same 2s deadline against the same 500ms ticker. It asserts on the `CRDs not established` wrapper the caller adds, and both messages carried that wrapper, so this coin flip could not reach it and nothing here touches it. I left the 2s deadline where it is. Moving it off the tick boundary would have hidden the coin flip and left the second message in place. The new tests catch a reintroduced second message deterministically, which is what that job now rests on rather than the timeout test's roughly one-in-sixty. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(manifestutil): a CRD wait cut short by a cancelled context now reports one error naming the CRD it was waiting on, where before the name was present or missing depending on which of two exits the scheduler happened to pick ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved cancellation handling while waiting for custom resources to become established. * Cancelled operations now stop before issuing unnecessary requests and identify the affected resource. * Timeout errors remain distinguishable from cancellation errors. * **Tests** * Added coverage for cancellation before, during, after, and between polling attempts. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
9ace63b441 |
fix(e2e): stop the sandbox cluster CIDR overlapping the pod range
The sandbox Talos config and the platform install step both used 10.244.0.0/16, one as the Kubernetes cluster CIDR and the other as the kube-ovn POD_CIDR, so two allocators drew from one range. kube-ovn owns pod IPAM and allocates flat from POD_CIDR. Cilium is chained behind it and allocates no pod addresses, but it still carves its own per-node router IP and Ingress IP out of Node.spec.podCIDR, which kube-controller-manager slices from the cluster CIDR. Sooner or later kube-ovn hands a pod an address cilium already holds. When that address is the Ingress IP the agent refuses the sandbox with "IP ipv4:<addr> is already in use", and cilium never relents on it, so the pod only proceeds if something reschedules it onto a different address. In the failure this comes from, the same address was refused seven times across six pod UIDs before the suite ran out of budget. When it is the router IP nothing complains at all and only host-to-pod traffic breaks. Move the cluster CIDR to 100.65.0.0/16, which no other range in the tree uses. That deliberately stops the sandbox matching the layout the install documentation prescribes, so it is not a value to put back. Pod addresses are unaffected: they still come from kube-ovn out of 10.244.0.0/16, and cilium does not require a chained endpoint's address to sit inside its own allocation CIDR. Excluding the addresses on the kube-ovn side would not work, because cilium picks its offsets by random scan over the node's range. The new guard checks the invariant rather than the literals, so either value may move as long as they stay disjoint. It covers the service and join ranges too, since landing on one of those instead is the next way to get this wrong. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Andrey Kolkov
|
d0990299ed |
feat(clickhouse): let the backup sidecar trust a private S3 CA, unblocking the backup e2e round-trip (#3385)
## What this PR does Gives the ClickHouse `clickhouse-backup` sidecar a way to trust an S3 endpoint whose certificate is signed by a private CA, and uses that to make the ClickHouse backup/restore e2e round-trip actually run in CI. This actualizes #2600, whose feature code and example scripts have since landed on `main` independently, and whose remaining contribution — the e2e test — was still in the retired BATS format. #2600 can be closed once this merges. ### 1. `backup.endpointCA` on the ClickHouse chart The sidecar had **no TLS/CA surface at all**: bucket coordinates and credentials arrive as `S3_*` env, and nothing let it verify an endpoint signed by a private CA. Cozystack's own in-cluster SeaweedFS is exactly that case — it serves `:8333` behind the self-signed "SeaweedFS CA" — so a ClickHouse release could only ever back up to a publicly-trusted endpoint. MariaDB (`storage.s3.tls.caSecretKeyRef`) and Postgres (barman `endpointCA`) have had a CA field all along; this closes the gap for ClickHouse. `backup.endpointCA` takes `{name, key}`. When set, the chart mounts that Secret into the sidecar and points `SSL_CERT_DIR` at it. Empty — the default — renders no env, no mount and no volume, so existing releases are byte-for-byte unchanged. **Why `SSL_CERT_DIR` and not `AWS_CA_BUNDLE`.** Both are honoured by `altinity/clickhouse-backup:2.7.4`; I measured them against a self-signed endpoint and public AWS S3 in turn. Go reads `SSL_CERT_DIR` *in addition to* the system bundle, so both endpoints verify. `AWS_CA_BUNDLE` *replaces* the SDK's trust pool: the private endpoint verifies, and public S3 then fails `x509: certificate signed by unknown authority` — it would silently break a release that also talks to a public endpoint. `S3_DISABLE_CERT_VERIFICATION` was rejected outright, since it drops verification rather than trusting one more CA. ### 2. The e2e suite, and what it was really testing Two Tests added to the existing `hack/e2e-chainsaw/clickhouse/` suite (in name order, mirroring the `etcd`/`mariadb`/`postgres` shape — see the layout rationale below for why they live here rather than in a new directory): - **`clickhouse-1-backup-contracts`** (CI, `tenant-test`, ~98s): a backup-enabled ClickHouse asserted against the *running* cluster — the `clickhouse-backup` sidecar on the `ch-backup-api` `:7171` port, and the `<release>-backup-api-auth` Secret whose keys the sidecar's `API_USERNAME`/`API_PASSWORD` actually resolve `valueFrom`. Each is a silent break if a chart bump renames it. Needs no object store. - **`clickhouse-2-backup-roundtrip`** (CI, `tenant-root`): `Bucket` → `Altinity` strategy + `BackupClass` → source ClickHouse with a sentinel row → `BackupJob` → in-place `RestoreJob` → to-copy `RestoreJob` into a second instance, driving `examples/backups/clickhouse/run-all.sh` so test and docs cannot drift. The round-trip was originally gated behind `CLICKHOUSE_E2E_S3_ROUNDTRIP` and **skipped in CI** — its `PASS (1.01s)` meant the gate fired, not that a backup happened. `backup.endpointCA` removes the reason for the gate, so the suite now adopts the same three preconditions mariadb/postgres rely on: run in `tenant-root` (an isolated tenant's Cilium egress allowlist blocks its Pods from reaching `tenant-root`'s SeaweedFS), target the in-cluster endpoint instead of the `Bucket`'s external ingress URL (an unroutable placeholder in CI), and trust its certificate via the copied CA. It also pre-cleans with `cleanup.sh`, because `tenant-root` is shared and a leftover `Succeeded` `BackupJob` would satisfy the harness's wait without a backup ever running. Both backup Tests live in the existing `hack/e2e-chainsaw/clickhouse/` suite rather than a new suite directory, so no selector mapping is added: `hack/select-e2e.sh` already derives `clickhouse` both from `packages/apps/clickhouse/**` (the `*-application` rule) and from `examples/backups/clickhouse/**` (the harness path), and both paths therefore select the suite that actually runs the harness. That alignment is the reason for the layout — a separate `clickhouse-backup/` directory would have made an edit to the harness select a suite that cannot exercise it. `hack/select-e2e.sh` and `hack/select-install.sh` are otherwise untouched (the only change is a comment recording that invariant), and the existing `backup example harness edit selects its app suite` case in `hack/select-e2e_test.bats` covers the mapping. ### Verification - `helm unittest` — 17 pass, including 5 new `endpointCA` cases: the unset default renders nothing, the env/mount/volume wiring, a custom key still projected as `ca.crt`, composition with `useSystemBucket`, and `backup.enabled=false` renders nothing. - The CA-trust behaviour was measured against the real image, not assumed: without a CA knob the sidecar reports `x509: certificate signed by unknown authority`; with `SSL_CERT_DIR` the handshake succeeds. - `hack/select-e2e_test.bats` (16) and `hack/select-install_test.bats` (17) pass; `select-install.sh --validate` reports graph OK. - `make generate` is idempotent, so the pre-commit job's `git diff --exit-code` stays clean. - The contracts Test passed in CI on earlier runs of this branch. The round-trip only became CI-live with this work, so it still needs a green E2E run before merge — the branch has since been rebased onto current `main` to get one. ### Screenshots Not applicable — no UI changes. ### Downstream repositories Walked the trigger map against the diff, file by file. The load-bearing change is the new `backup.endpointCA` field in `packages/apps/clickhouse/values.yaml` / `values.schema.json`; everything else is `hack/e2e-chainsaw/`, `examples/backups/`, one docs paragraph, and regenerated artifacts, none of which appear in the map. - [ ] No downstream repository is affected by this change - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: https://github.com/cozystack/terraform-provider-cozystack/issues/18 — "add, remove or rename a field in an app's `values.schema.json`" reaches the provider's hand-maintained schema, model and expand/flatten pair, so the new nested object is not surfaced until it is added there. Opened as an issue rather than a PR because the natural moment is alongside that repo's next `api/apps/v1alpha1` pin bump, which is a maintainer call. Not ticked, with reasons rather than silence: **cozystack/website** — the trigger is *adding, renaming or removing a package*, and this adds a field to an existing one. `clickhouse` is already in the website `Makefile`'s app list and its reference page is generated from this package's `README.md`, so the regenerated `backup.endpointCA` rows are carried by the existing release-tag automation with no manual PR. **cozystack/external-apps-example** — vendors `hack/package.mk` and `update-crd.sh`, neither of which this PR touches. **cozystack/ansible-cozystack**, **ccp**, **talm**, **cozyhr**, **cozy-proxy**, **cozystack-telemetry-server**, **examples** — no installer values, `hack/` layout, namespace, variant, node-prerequisite, annotation, label or metric in their trigger rows is touched here. ### Release note ```release-note feat(clickhouse): add `backup.endpointCA` so the clickhouse-backup sidecar can verify an S3 endpoint whose certificate is signed by a private CA, such as Cozystack's in-cluster SeaweedFS. Leave it unset for publicly-trusted endpoints; existing releases are unaffected. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added optional private S3 endpoint CA support for ClickHouse backup/restore sidecars via `backup.endpointCA`, including configurable Secret name and CA bundle key (default `ca.crt`). * Sidecars mount the CA and extend trust while leaving public endpoints unchanged. * **Documentation** * Updated ClickHouse chart docs, operational backup guidance, and backup/restore examples to cover CA setup, overrides, and cleanup. * **Tests** * Added helm-unittest and end-to-end coverage validating CA rendering/mounting behavior and ClickHouse backup/restore workflows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
24157ec876 |
chore(codeowners): keep architects as owners in every non-governance rule (#3738)
## What this PR does CODEOWNERS applies only the **last** matching rule, and matching is not additive: a scoped rule fully replaces the `*` catch-all instead of extending it. As a result `@kvaps` and `@lllamnyp` were not code owners for most scoped paths (21 and 36 of the 76 owner-bearing rules, respectively), and several paths (`/hack/`, `/.github/`, `/Makefile`, `/.pre-commit-config.yaml`, `/docs/`) were owned by exactly two people — with "Require review from Code Owners" enforced on `main`, a PR authored by one of them could only be approved by the other. This PR: - appends `@kvaps` and `@lllamnyp` to every rule that lists owners, keeping existing owners and their order; the governance block deliberately stays with `@tym83 @kvaps`, and the ownerless rules for generated files stay ownerless; - spells out the non-additive last-match semantics in the file header; - adds `hack/codeowners-invariant.bats` (auto-discovered by `make bats-unit-tests`), so a future scoped rule that omits the catch-all owners fails unit tests instead of silently carving its path out from under them. ### 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: ### Release note ```release-note chore(codeowners): @kvaps and @lllamnyp are now code owners for every path except governance files, removing single-approver review bottlenecks; a unit test guards the invariant ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated code ownership rules across dashboards, packages, Kubernetes components, backups, GPU and SeaweedFS components, Keycloak, documentation, and CI files. * Expanded area-specific review ownership while preserving existing governance and API review assignments. * **Tests** * Added automated validation to ensure ownership rules include the required default reviewers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
5747747087 |
fix(manifestutil): report one message when the CRD wait is cancelled
WaitForCRDsEstablished reported a cancelled context two different ways. The check at the top of the poll loop returned a message with no CRD name; the select at the bottom named the CRD the poll had stopped on. Which one ran was left to the scheduler: when the deadline and a tick come ready in the same instant, a select picks between the ready cases at random, so one cancellation reached the operator either way. Build the message in one place and return it from both exits, so the two are indistinguishable by construction. The name is carried across iterations, since a cancellation noticed at the top of the loop has no poll of its own to take one from, and starts as the first CRD of the set for a wait cancelled before any poll observed anything. Keeping the check at the top is what makes carrying the name worthwhile. A client refuses Get on a dead context at the first name in the list whichever CRD is actually lagging, so a loop that re-entered and polled anyway would overwrite the name its last poll settled on. The check closes that window and no more than that: a context dying part-way through a poll still records the first Get to fail, because this branch cannot tell a refused request from a CRD status. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
IvanHunters
|
cfaf2e086c |
fix(backupstrategy-controller): repair lookup-gated backup objects (#3524)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does Fixes #3518. Objects in `packages/system/backupstrategy-controller` and `packages/system/bucket` are gated on a Helm `lookup` of an object that is still being created. When the lookup is empty the object is skipped — and since helm-controller does not re-render a release whose chart and values did not change (drift detection is off on operator-generated HelmReleases), **the skip is permanent**, not a bootstrap window. On the reporter's cluster the install-timing window was 4 seconds and the objects were still missing months later: - the 7 default backup `Strategy` CRs and the Velero `BackupStorageLocation`, gated on `include "backupstrategy-controller.bucketName"` → a `lookup` of `BucketClaim`; - the `<bucket>-<user>-credentials` Secret (`packages/system/bucket/templates/user-credentials.yaml`), gated on a `lookup` of the COSI Secret. Only `BackupClass cozy-default` was present, because it renders unconditionally. Its own comment predicted this case while assuming "Flux re-renders the chart", which does not happen — the same trap `packages/core/platform/sources/cozystack-basics.yaml` already documents: *"Drift detection is off on operator-generated HelmReleases, so a capability gate in the templates would render the policy out at first install and never add it back."* Consequence: migration 50 (etcd adoption, v1.6.0) is fail-closed on the snapshot target. It resolves the target from the projected credentials Secret, which has no source without `<bucket>-<user>-credentials`, so its Job reboots forever and the cluster cannot be upgraded at all. ### 1. `backupstrategy-controller` reconciles the objects into existence New leader-elected `DefaultObjectsGate` runnable (`internal/backupcontroller/default_objects_gate.go`), modelled on the existing `SystemCredentialsProjector`. Every minute: - resolves the bucket name from the **projector's source Secret** (`parseSourceSecret`) — no new dependency, no `lookup`, and it covers the external-S3 path too. While it is unresolvable the gate does nothing: forcing then would only re-run the same empty lookup; - reads `BackupClass cozy-default` as the manifest of what must exist (its `strategyRef`s), plus the `cozy-default` BSL when `velero.bslEnabled`; - when any is missing, stamps `reconcile.fluxcd.io/forceAt` **and** `requestedAt` on its own HelmRelease. Both are required: `requestedAt` alone only requests a reconcile, which is a no-op for an unchanged release; `forceAt` is what triggers a real Helm upgrade, and it is only honoured together with `requestedAt`; - throttled to one forced upgrade per 5 minutes, and a no-op in the steady state. A `strategyRef` whose CRD is not installed is not counted as missing, so a missing CRD cannot drive an endless force loop. Two new metrics: `cozystack_backup_default_objects_missing{backupclass}` — the alert that would have caught this, since it is non-zero regardless of the HelmRelease's `Ready` condition — and `cozystack_backup_default_objects_force_reconciles_total`. RBAC delta is two lines: `patch` on `helmreleases` (`get` was already there) and `get` on `backupstoragelocations`. Both accesses are point Gets through the dynamic client, so no cluster-wide informer and no `list`/`watch`. Off switch: `backupStorage.reconcileDefaultObjects: false`. **Why not a render-time `fail` in this chart:** it would deadlock. `templates/bucket.yaml` in this same chart is the producer of the `Bucket` whose `BucketClaim` the lookup reads, so a failed render never applies it and the condition can never resolve. **Why not template the objects in Go:** their bodies are values-driven (endpoint, region, `forcePathStyle`, the Altinity strategy's whole PodTemplateSpec). Helm stays their single author; the gate only makes sure the render that produces them actually happens. **Why not `dependsOn`:** an edge guarantees the `Bucket` CR is *applied*, not that its claim is *reconciled*, so it does not close the race. ### 2. The gate also repairs the credentials Secret, one release earlier The `<bucket>-<user>-credentials` Secret is the same trap as the Strategy CRs, one release up the chain: `packages/system/bucket/templates/user-credentials.yaml` renders it behind a `lookup` of the COSI Secret, and skips it just as permanently. It is also the *first* domino — the projector reads it, so without it there is no `cozy-backups-creds`, no strategy, no Velero, and no migration 50. An earlier revision of this PR made that render `fail` so the release would retry and converge. [@IvanHunters pointed out](https://github.com/cozystack/cozystack/pull/3524#issuecomment-5214269078) that Helm cannot render a partial set, so the `fail` aborts the **whole** `<bucket>-system` release — the other users' Secrets and the bucket UI Deployment/Service/Ingress/HTTPRoute with them. One declared user whose `BucketAccess` never provisions would take down every other user of that bucket, and on an installed release park it in `Failed`, blocking every later upgrade. That was a real blast-radius regression on a universal path, and it is gone. The repair now lives in the gate, where it is per-object: - while the source Secret is absent, or carries no bucket name, the gate stamps `forceAt` + `requestedAt` on the bucket's `bucket-<name>-system` HelmRelease instead of doing nothing; - **no self-deadlock:** the `BucketClaim` and the `BucketAccess` whose COSI Secret that lookup reads are rendered unconditionally by the **parent** release (`packages/apps/bucket/templates/bucketclaim.yaml`), not by the one being forced; - **no RBAC delta:** `patch` on `helmreleases` is already cluster-scoped; - the two releases are throttled **independently**. They are forced in sequence on a bootstrap (credentials, then the objects the resolved bucket unblocks), so one shared timestamp would delay the second by a full `MinForceInterval` for no reason; - coordinates come from the chart under `provisionBucket`, and are omitted on external S3, where the Secret is admin-managed and no release renders it. The bucket chart keeps its partial render. Its comment now explains why the skip is not self-healing and why failing is the wrong lever, and a test renders the UI with *every* user unresolvable so a reintroduced `fail` fails the suite. **Scope this narrows, deliberately:** tenant buckets keep the pre-existing silent skip. The gate owns only the platform bucket, which is the cluster-breaking path. Generalising the repair to every bucket needs a controller that owns tenant buckets and is a separate change. Two more corrections from the same review: - `cozystack_backup_default_objects_missing` was only written on the happy path, so the state this PR exists to catch reported `0` and the documented alert never fired. The credentials Secret now counts in `missing`, so an absent or rotated-away Secret moves the gauge. On an API error the gauge is deliberately left alone rather than flapping, and a new `cozystack_backup_default_objects_check_errors_total` marks it stale; the doc pairs the two instead of overstating the gauge. `force_reconciles_total` gained `namespace`/`name`, since two releases can now be forced. - `forceHelmRelease` does a point `Get` and skips a release with `spec.suspend: true`. helm-controller ignores both annotations while suspended (`cozyhr suspend` sets exactly that), so the gate was re-stamping every `MinForceInterval` for the whole suspension and climbing the force counter — which the runbook attributes to a render that is not producing the objects. The skip is logged and not counted. ### 3. Corrected comments and docs `_helpers.tpl`, `backupclass-default.yaml`, `values.yaml` and `docs/operations/backup-classes.md` all claimed the skip self-heals on the HelmRelease interval, and the doc suggested a `flux reconcile` that does not re-render. They now say it does not, and why. The two-release manual recovery for already-affected clusters is documented, including the easy-to-miss part: the credentials Secret is rendered by the `bucket-<name>-system` release, not by `bucket-<name>`. ### Manual recovery for already-affected clusters Only needed on a version without the gate. A plain `flux reconcile helmrelease` does **nothing** — it does not re-render. ```bash # 0. Confirm the bucket name is resolvable first — forcing before that # just re-runs the same empty lookup. kubectl -n tenant-root get bucketclaim bucket-cozy-backups -o jsonpath='{.status.bucketName}' # 1. The credentials Secret. Rendered by the -system release, NOT by # bucket-cozy-backups — easy to miss, and the projector (hence every # strategy, Velero, and migration 50) has no source without it. ts=$(date -u +%Y-%m-%dT%H:%M:%SZ) kubectl -n tenant-root annotate helmrelease bucket-cozy-backups-system \ reconcile.fluxcd.io/forceAt="$ts" reconcile.fluxcd.io/requestedAt="$ts" --overwrite # 2. The Strategy CRs and the Velero BSL. ts=$(date -u +%Y-%m-%dT%H:%M:%SZ) kubectl -n cozy-backup-controller annotate helmrelease backupstrategy-controller \ reconcile.fluxcd.io/forceAt="$ts" reconcile.fluxcd.io/requestedAt="$ts" --overwrite ``` ### Validation - `go test ./internal/... -count=1` — pass, including 15 cases for the gate: force on missing, no-op in the steady state, throttling, unmapped kinds ignored, BSL skipped when disabled, patch failure surfaced without advancing the throttle, absent HelmRelease tolerated, plus force-on-absent-Secret, force-on-empty-bucket-name, external-S3 reports-but-does-not-force, independent throttles, suspend skip on each of the two releases, gauge written on the unresolved path, and errors counter on a failed check. - `make helm-unit-tests` — pass, with 2 new suites: 3 cases for the bucket chart (partial render preserved, and the UI still renders with every user unresolvable — the guard against reintroducing the `fail`), 7 for the gate's env + RBAC wiring. Those include that `.Release.Name` and `.Release.Namespace` match the HelmRelease the operator generates (`releaseName` becomes the HR's `metadata.name`, and no `spec.releaseName`/`targetNamespace` is set, so the Helm release name equals the HR name), that the bucket release coordinates track a renamed bucket together with `systemSecretName`, and that they are unwired on `provisionBucket: false`. - `make generate` — no diff. - `make go-unit-tests rd-presets-check test-check-readiness migrations-target-check` — pass. - `make bats-unit-tests` — every suite passes except `hack/migration-seaweedfs-db-adopt.bats`, which needs a Docker daemon not available in this environment ("docker is required: these tests run the migrations inside alpine"). Unrelated to this diff. **Not verified — please read this before approving.** There was still no live-cluster run. What that gap covers is smaller than it was: dropping the chart `fail` removed the universal behaviour change on every bucket's install path, so there is no longer a render-blind failure mode whose convergence has to be observed. What remains is one mechanism — `forceAt` + `requestedAt` on an unchanged release — applied to two releases instead of one, argued from read code rather than an observed install: `Strategy.Name = RetryOnFailure` on operator-generated HelmReleases (`internal/operator/package_reconciler.go`), and `bucketName = "bucket-" + claim.UID` in `packages/system/objectstorage-controller/images/objectstorage/patches/92-bucketclaim-propagate-ready.diff`. It is the same procedure as the manual recovery below, which has been run by hand on the reporter's cluster. Someone with a bootstrap cluster should still confirm that the gate fires within a minute or two of the bucket becoming ready, and that forcing `bucket-<name>-system` materialises the credentials Secret. ### Screenshots Not applicable: no UI change. This PR touches a controller, two system charts, and docs. <!-- 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. --> Walked file by file against the diff of this PR. **`website` — affected, follow-up issue opened.** `content/en/docs/next/operations/services/backup-classes.md` mirrors this repo's `docs/operations/backup-classes.md` and carries the exact stale claim this PR corrects, plus a `flux reconcile helmrelease` recovery suggestion that has no effect. `operations/configuration/platform-package.md` enumerates the forwarded `backupStorage` keys, and `reconcileDefaultObjects` is admin-settable through `spec.components.platform.values.backupStorage` (the whole block is deep-merged), so that list becomes incomplete. Filed as an issue rather than a PR because the wording depends on the shape this PR lands in, and whether to touch the frozen `v1.5`/`v1.6` copies is a maintainer call. No existing website issue or PR covered it. **`terraform-provider-cozystack` — not affected.** Checked, not assumed: `packages/apps/**` is untouched by this diff (`git show --stat HEAD -- packages/apps/` is empty), so no app `values.schema.json`, no `values.yaml` default, no version enum, and no `ApplicationDefinition` `kind`/`plural`/`release.prefix` changed. `reconcileDefaultObjects` is a key on a **system** chart, which has no `values.schema.json` and is not modelled by the provider. (An earlier revision added `requireUserCredentials` to the `bucket` system chart; that is gone, and no key was added to `apps/bucket`, so the app API is untouched.) The `<bucket>-<user>-credentials` Secret the provider looks up by literal name is **not renamed** — this PR only changes whether it gets created, which if anything removes a case where the provider silently read null from a Secret that never existed. Everything else: no `hack/` layout or make-target change (`ccp`, `external-apps-example`, `cozyhr`), no `ApplicationDefinition` CRD change, no namespace rename, no node prerequisites in `hack/e2e-prepare-cluster.bats` (`talm`, `ansible-cozystack`), no `cozyhr.cozystack.io/values-files` annotation or chart-source-kind change, no `service-proxy-name` / `wholeIP` / `allowICMP` rename (`cozy-proxy`), no telemetry metric or label rename (`cozystack-telemetry-server` — the two metrics here are new, not renamed), no node or network requirement change (`examples`). - [ ] No downstream repository is affected by this change - [x] [cozystack/website](https://github.com/cozystack/website) - follow-up: https://github.com/cozystack/website/issues/639 - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(backupstrategy-controller): the default backup Strategy CRs, the Velero BackupStorageLocation and the bucket user-credentials Secret were skipped permanently when their install-time Helm `lookup` came back empty, leaving clusters with a BackupClass but no strategies and blocking the v1.6.0 etcd migration. The controller now detects the missing objects and forces the Helm upgrade that creates them, including on the bucket release that renders the credentials Secret. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added automatic detection and recovery for missing default backup resources. - Missing resources trigger a throttled Helm upgrade once bucket configuration is available. - Added configuration to enable or disable default-object reconciliation. - Added validation requiring user credential Secrets during bucket installation by default, with an offline rendering option. - **Bug Fixes** - Improved recovery from incomplete backup and Velero storage configuration. - **Documentation** - Expanded operational guidance, monitoring metrics, alerts, and manual recovery steps. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
980715250c |
fix(kube-ovn): reload kubeovn-webhook serving certificate on cert-manager renewal (#3557)
## What this PR does The `kube-ovn-webhook` loaded its TLS serving certificate once at process startup (`tls.LoadX509KeyPair` into a static `tls.Config.Certificates`) and never re-read it. cert-manager renews the backing Secret ahead of expiry, but a running pod kept serving the certificate it loaded at boot. Once that certificate expired — roughly one year after install, on any cluster whose webhook pods had not been restarted after the renewal — the pod served an expired certificate, the kube-apiserver's TLS call to the webhook failed verification, and because the `MutatingWebhookConfiguration` uses `failurePolicy: Fail`, every pod creation in tenant namespaces was rejected. In practice this blocks `virt-launcher` pods, so no VMI can start. This PR: - Serves the certificate through a reloading `tls.Config.GetCertificate` callback that re-reads the key pair when the mounted files change, and keeps serving the last good certificate if a reload fails (e.g. a torn read while the volume is being updated). cert-manager renewals are now honoured without a pod restart. - Widens the certificate `renewBefore` from `24h` to `720h`, so renewal happens well ahead of expiry instead of one day before. - Adds a regression test that fails against the previous static implementation (the server keeps presenting the old certificate after the files are replaced) and passes once the certificate is reloaded; verified non-vacuous by mutation. ### Screenshots Not applicable (no UI changes). ### Downstream repositories - [x] No downstream repository is affected by this change Walked the trigger map file-by-file against the diff: the change is confined to the `system/kubeovn-webhook` image and its cert-manager `Certificate` `renewBefore`; it touches no package under `apps/`/`extra/`, no `values.schema.json`, no app `values.yaml` default, no `ApplicationDefinition`, no platform values, no CRD, and no `hack/` tooling. ### Release note ```release-note fix(kube-ovn): kubeovn-webhook now reloads its serving certificate when cert-manager renews it, so the webhook no longer serves an expired certificate that blocked all pod creation (including VMIs) roughly one year after install ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Webhook TLS certificates now reload automatically when renewed, without requiring a server restart. * **Bug Fixes** * Certificate reload failures now fail the TLS handshake instead of serving an outdated certificate. * Added server timeouts to improve connection handling. * **Configuration** * Certificates are now renewed earlier to help prevent expiration-related service disruptions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
efa7f5c8fb |
fix(velero): apply CRD updates on upgrade via CreateReplace (#3727)
## What this PR does Velero CRDs stayed frozen at whatever version was first installed: Helm never touches the chart's `crds/` directory on upgrade, the generated HelmRelease left `spec.upgrade.crds` at the helm-controller default (Skip), and the package disables the upstream upgrade-crds Job. When the velero image moved to v1.18.1, which added the `Queued` and `ReadyToStart` backup phases, the apiserver rejected every phase transition against the stale CRDs (`status.phase: Unsupported value: "Queued"`) and backups silently stopped while the HelmRelease stayed green. This opts the velero PackageSource into `upgradeCRDs: CreateReplace`, the same policy mongodb-operator already uses, adds a platform test pinning it, and corrects the stale Job-disable rationale in the package. CreateReplace is safe here: velero only extends the phase enums between versions. Closes #3726 ### 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: ### Release note ```release-note fix(velero): apply Velero CRD updates on upgrade; previously CRDs stayed at their first-install version, so after the image moved to v1.18.1 backups could silently stop with `status.phase: Unsupported value: "Queued"` while the HelmRelease stayed green ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Velero now automatically creates or replaces its Custom Resource Definitions during installation and upgrades. * **Bug Fixes** * Improved Velero CRD lifecycle handling to ensure updated definitions are applied reliably. * **Tests** * Added coverage verifying the CRD upgrade configuration and updated validation guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
f4d812cfb9 |
fix(velero): apply CRD updates on upgrade via CreateReplace
velero evolves its CRD schema between versions; v1.18 adds the Queued and ReadyToStart backup phases. Helm never upgrades CRDs shipped in crds/, the generated HelmRelease left spec.upgrade.crds at the Skip default, and the package disables the chart's upgrade-crds Job, so live CRDs stayed frozen at first-install state: the server upgrades, the apiserver rejects the new phases, and backups sit in New while the HelmRelease stays green. Opt the velero PackageSource into CreateReplace, consistent with mongodb-operator, add a platform test pinning the policy, and correct the stale Job-disable rationale in the package. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
f0bde76b9f |
docs(hack): fix a wrong claim about grep-error propagation in two contract tests
Both hack/promote-gate-contract.bats and hack/release-freeze-contract.bats carried the same code_lines helper with the same wrong comment: it claimed a real grep error fails the pin through the helper's own exit status. code_lines is never the last stage of a pipeline in either file, so ordinary (non-pipefail) pipe semantics discard whatever it returns on its own, and hack/cozytest.sh's translator appends a return 0 to every line that is exactly }, code_lines' closing brace included, making that exit status even less trustworthy as a signal. What catches a real grep error is the output, and only for one class of pin. The error empties code_lines' stream, so a pin that requires something to be present in that stream fails on it. A pin demanding an absence stays blind: an exact count of zero is what an emptied stream produces anyway, and what the pin was written to assert. Both files hold pins of that shape. The per-test [ -n "$block" ] guard is not the backstop either. In these two files a block is a bare awk extraction, upstream of code_lines, so that guard cannot see the filter fail; the comment now says so, because reading it as the backstop gets the direction exactly wrong. Signed-off-by: Aleksei Sviridkin <f@lex.la> Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <3811295@gmail.com> |
||
|
Aleksei Sviridkin
|
ccd8a289ef |
fix(e2e): budget each linstor wait separately in post-install-prep (#3715)
## What this PR does Both waits in `hack/e2e-post-install-prep.sh` ran off one 15-minute deadline anchored at the script's start, so the head of the chain ate the window and the tail got whatever was left. Two runs on 2026-08-09 failed that way. The linstor HelmRelease went Ready 452s in, and the Deployment needed a further ~520s because its DB migration lost the apiserver and retried four times. Neither figure passes 900s alone. Their sum does, and the install reported the Deployment as broken. Each wait now gets its own budget, started when the previous link finished. Same shape as the node wait and the MetalLB CRD wait further down the same file. What it buys is that every link gets its whole allowance regardless of what the links ahead of it spent. The cost is on both sides and I measured it. Worst case goes from one 15m window to 900 + 900 + 300 + 300 = 40m, against the 180m the e2e job carries. A Deployment that never converges is now reported at 1355s instead of 900s, from the 452s above plus the loop's 5s poll. A HelmRelease that never goes Ready is still reported at 900s, which I checked by running the new suite against the pre-change script. The timeout now dumps the pods and recent events of `cozy-linstor`. The same "Deployment not Available" has come from a crash-looping migration init container and from a pod waiting on a client TLS Secret cert-manager had not issued yet, and telling those apart used to need the run's bundle. Events come from `kubectl events`, which sorts by last-seen and carries no `--sort-by` to fail on a key some item lacks. Both reads keep their stderr and are bounded, with the client budget smaller than the outer one so kubectl gets to say why before the kill lands. `hack/post-install-prep.bats` runs the script end to end against a stub PATH with a virtual clock, so a 15-minute budget elapses in a few hundred forks and the timings are exact. Four of its six tests fail against the pre-change script. The other two guard the bound that stays. ### Known gaps in the new tests Dropping the `2>&1` from either diagnostic read leaves all six tests green, because the harness catches the script's own stderr into the same file the assertion reads. The property holds in production anyway, since the caller merges both streams into one log, but nothing catches the removal. The test for "one failing read does not suppress the other" needs the pods read to run first; swap the two reads and it passes vacuously. Its comment says so at the point where someone would make that change. ### 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: Walked the map against the diff. Two files, both under `hack/`. The `ccp` trigger is about moving or renaming things under `hack/` or changing what a make target does, and its anchors `hack/package.mk` and `hack/common-envs.mk` are untouched. `talm` and `ansible-cozystack` key off node prerequisites in `hack/e2e-prepare-cluster.bats`, a different file. ### Related Same family, found while working here and filed instead of folded in. #3707 and #3708 for unbounded reads elsewhere in this script, #3712 for the `--sort-by=.lastTimestamp` form in fourteen other places, #3713 for the wait loop dropping kubectl's stderr for its whole budget, #3714 for the MetalLB CRD wait failing without a message. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved LINSTOR readiness checks by giving each prerequisite its own 15-minute wait period. * Added clearer timeout diagnostics, including relevant pod and event information. * Ensured subsequent readiness checks are skipped when an earlier HelmRelease check fails. * **Tests** * Added end-to-end coverage for timeout handling, sequential failures, diagnostic errors, and readiness behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
6ee56e3a60 |
fix(ci): decide E2E test coverage per changed path (#3441)
## What this PR does
`hack/select-e2e.sh` escalates to the full Chainsaw suite when a changed
package reaches no suite through the dependency graph, because otherwise
that change gets no E2E coverage at all. The escalation was read off the
merged selection, so it depended on the whole diff rather than on the
path that needed it. Any other changed path contributing a suite name
made the selection non-empty and the escalation vanished with nothing in
the output to say it had. Editing a platform component together with one
unrelated suite's Chainsaw test ran that single suite instead of
everything, which is an ordinary PR shape: change a component, adjust
the e2e near it.
Coverage is now decided per changed path. Each path resolves through its
owning sources to the suites that exist, and a path reaching none of
them forces the full run whatever the rest of the diff selected. The
unit is the path rather than the source, because
`system/postgres-operator` belongs to two PackageSources where one
reaches no suite, and deciding per source would run everything on every
change to it.
The other half is the mapping that left packages unreachable from their
own suites. `src_to_suites()` covered `*-application` names plus
`external-dns`, and the final filter dropped the rest, so
`cozystack.kuberture` and `cozystack.securitygroup-controller` were
walked to and then discarded. A CNI change ran neither of the two
controller suites most likely to regress from it, and a change to either
package escalated to the full run rather than selecting the suite that
covers it. Every reached source now maps through the table, which is the
inverse of `select-install.sh`'s `suite_to_source()`; a test walks every
suite through both, so the next source name fitting neither convention
fails there instead of quietly escalating forever.
## What was measured
Behaviour was diffed against the current selector over one `values.yaml`
per package under `packages/{apps,system,extra}`: 159 packages, 8
selections change, all of them the mapping repair. Five networking
packages and `prometheus-operator-crds` widen — 19 to 21 suites and 10
to 11, both gaining `kuberture` and `securitygroup`. `kuberture` and
`securitygroup-controller` narrow from the full suite to their own
single suite. Nothing narrows that should not.
That sweep is one path per diff, so it says nothing about the per-path
escalation, which only shows on a diff with more than one path. Pairing
each package with one unrelated Chainsaw edit gives that number. 82 of
the 159 packages escalate on their own and stop escalating once the pair
is added, which is the size of the hole: for those, adding an unrelated
suite edit to the same pull request replaced the full run with one
suite. 88 packages select something different after this change, the
extra six being the mapping repair widening a selection rather than
restoring an escalation.
Worth stating alongside it, because it decides whether the branch is a
corner or the common case: 83 of the 159 escalate on a single-path diff
today, and 81 do after this change — the two that leave are `kuberture`
and `securitygroup-controller`, which now select the suite that covers
them. So the escalation is the branch most packages take, and the
swallowed one was the branch most packages took. Counting that needs an
instrument that separates a graph walk which reached everything from an
escalation which printed everything, since the two agree once the
mapping is fixed: the count above comes from a scratch suite tree
carrying one extra suite that no source names, which only an escalation
can emit.
The suite selection is what decides which tests run, so a rule that
stops firing does not go red, it disappears. The tests pin the property
rather than a worked example: that a path reaching no suite escalates
whatever else the diff holds, that a covered path never drags the full
suite in with it, and that every suite round-trips between the two
mapping tables. Each was checked by mutation — removing the rule it
guards turns it red.
## Left for a follow-up
`resolve_suites` writes into the caller's scope because POSIX sh has no
`local`, and what keeps that safe is the single call site sitting inside
`$( )`, pinned by a test that counts occurrences of the name. Declaring
the function with `( … )` instead of `{ … }` would contain those
variables by construction and let both the test and the two paragraphs
explaining the constraint go away. A count-the-name grep is the weakest
kind of pin and it exists only because the function leaks.
The output contract at the top of the script lists the causes of a
full-suite selection but not the backstop, which after this change is a
mechanism of its own: an edit under `hack/e2e-chainsaw/<dir>/` where
that directory holds no `chainsaw-test.yaml`. The behaviour is pinned by
a test and explained where it lives; only the header block is short. In
the same area, the escalation line names the owning sources rather than
the changed path, and reports the first uncovered group rather than all
of them.
## Overlap
#3434 fixes the same escalation from a different angle: it decides the
escalation from the whole source-derived selection rather than per path.
That closes the "package with no coverage plus an unrelated suite edit"
shape, and leaves "package with no coverage plus another package that
does select suites", where the source-derived selection is non-empty and
the escalation is swallowed again. There is a test for that second shape
here.
### Screenshots
Not applicable, no UI changes.
### Downstream repositories
Walked the trigger map in `docs/agents/contributing.md` against the
diff. The changed files are `hack/select-e2e.sh`,
`hack/select-e2e_test.bats` and `docs/agents/e2e-testing.md`. The only
`hack/`-related trigger is cozystack/ccp, which gates on
`hack/package.mk`, `hack/common-envs.mk` and `hack/update-crd.sh` and on
what a make target does; nothing here is moved, renamed, or changed in
make-target behaviour. The ansible-cozystack and talm triggers are about
`hack/e2e-prepare-cluster.bats`, which is untouched.
- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
### Release note
```release-note
fix(ci): a changed package that no E2E suite covers now runs the full suite even when the same pull request also edits an unrelated suite, and changes to the kuberture and securitygroup packages select their own suite instead of escalating
```
Closes #3330
Closes #3665
|
||
|
Andrei Kvapil
|
361c729417 |
test(codeowners): guard the coverage invariant with a unit check
CODEOWNERS is last-match-wins, so a new scoped rule that omits the default owners silently strips their ownership for that path. Add a hack/*.bats unit test (auto-discovered by make bats-unit-tests) that requires every rule listing owners to repeat the owners of the * catch-all, exempting ownerless rules and the governance roster. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Andrei Kvapil
|
bd7b76691e |
chore(codeowners): keep architects as owners in every non-governance rule
CODEOWNERS applies only the last matching pattern, so a scoped rule replaces the * catch-all instead of extending it: @kvaps and @lllamnyp were not code owners for most scoped paths, and several paths (/hack/, /.github/, /Makefile, /.pre-commit-config.yaml, /docs/) were owned by exactly two people, deadlocking PRs authored by either of them while the other is away. Append @kvaps and @lllamnyp to every rule that lists owners, except the governance block which deliberately stays with @tym83 @kvaps, and spell out the non-additive last-match semantics in the file header. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io> |
||
|
Aleksei Sviridkin
|
ba15b3817a |
fix(cozystack-basics): normalize tenant hostname VAP comparisons with lowerAscii
The Ingress, Gateway, and Route hostname ValidatingAdmissionPolicies compare tenant hostnames against the tenant apex with case-sensitive CEL. The resource-side host is already lowercase — the apiserver's hostname validation permits nothing else — but the apex operand is the namespace.cozystack.io/host label value, which Kubernetes permits to carry uppercase, and the Ingress policy additionally compares against the templated root host, a free-form platform value that nothing validates for case. A mixed-case apex label wrongly denies a tenant its own legitimate lowercase in-apex hostname. An uppercase root host is the dangerous direction: the Ingress outside-root exclusion compares a lowercase host against the uppercase literal, never matches, so the exclusion stops excluding and the policy degenerates into admitting any non-wildcard host — every non-wildcard platform-apex name a tenant asks for, including the root apex itself. Only the anti-wildcard gate still holds. Wrap both operands of every comparison in .lowerAscii() so the match follows DNS case semantics — no behavior change for the all-lowercase default. Each policy reads a label it does not own: the namespace host label is settable out-of-band by a trusted caller, so none of them may assume the apex arrives normalized. The Gateway policy admits any listener hostname when the host label is absent or empty, unlike its two siblings. That is the platform's one documented fail-open exception, recorded in the threat model together with its compensating control, so changing the posture is a security-model decision rather than a side effect of normalizing case. This leaves the behavior untouched and pins its current shape in the tests. Covered by helm-unittest assertions pinning each comparison as a whole operand pair, so dropping the normalization from any single operand is caught; a rendering case that pins the uppercase-root-host escape shut; and e2e probes that set a mixed-case apex on an isolated namespace and assert both halves of the match — a lowercase in-apex HTTPRoute and Ingress are still admitted, while a foreign apex and a hostname that ends with the apex without a dot boundary are still denied. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
86674c7084 |
fix(e2e): budget each linstor wait separately in post-install-prep
The post-install prep script waits out a serial reconcile chain -- the linstor HelmRelease, then the linstor-controller Deployment -- against a single 15-minute deadline anchored at the script's start. The head of the chain therefore eats the window and the tail inherits whatever remains, so an install fails whenever the chain merely runs long rather than never converging. Measured on one such failure: the HelmRelease went Ready 452s in, and the Deployment needed a further ~520s because the controller's DB migration lost its apiserver connection and retried four times before succeeding. Neither figure exceeds 900s on its own; only their sum did, and the install reported the Deployment as though it were broken. Give each wait its own budget, started when the link before it completed -- the shape the LINSTOR node and MetalLB CRD waits further down already have. What that buys is not speed but a guarantee: every link gets its whole allowance regardless of what the links ahead of it spent, so the tail can no longer be failed by the head being slow. It is a trade, and the other half is worth stating. The script's worst case becomes 900 + 900 + 300 + 300 = 40m against the 180m the job carries, where before the two waits shared one 15m window. And a link that is genuinely stuck is reported later rather than sooner: a Deployment that never converges surfaces at 1355s where the shared window surfaced it at 900s, on that run's timings. A HelmRelease that never becomes Ready surfaces at the same moment as before, since its budget still starts with the script. A timeout now also lists the linstor pods and the namespace events. The description names the object waited on, not the link that held it up, and the same "Deployment not Available" has been reached both with the controller pod crash-looping its migration init container and with that pod blocked on a client TLS Secret cert-manager had not issued yet. Telling those apart previously required the run's bundle. The events come from `kubectl events` rather than `kubectl get events`. It orders by when an event was last seen, so a condition repeating for minutes -- what a stuck mount looks like -- stays inside the window instead of being pushed out by newer one-off events, and it carries no --sort-by, so it cannot fail the whole read the way a sort key does when one item lacks it. Both reads keep their stderr and carry a bound. A read that fails silently is indistinguishable from a namespace with nothing to report, which is the distinction being drawn; and the caller is blocked in `wait`, so an unbounded read against a wedged apiserver would hold the install open until the job's own timeout. The client budget is strictly the smaller of the two so kubectl gets to name the reason before the outer kill takes it away. The wait helper is named for linstor because those diagnostics read one namespace unconditionally: the wait is generic over its kubectl arguments, what it dumps on the way out is not. The new unit test runs the script end to end against a stub PATH carrying a virtual clock, so a 15-minute budget elapses in a few hundred forks and the timings are exact rather than wall-clock approximate. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
587a7b9fbd |
fix(ci): decide E2E coverage per changed path
The full-suite escalation for a changed package that reaches no suite was
read off the merged selection, so it depended on the whole diff rather
than on the path that needed it. Any other changed path contributing a
suite name made the selection non-empty and the escalation vanished with
nothing in the output to record it: edit a platform component, adjust the
Chainsaw test beside it, and the component ran that single suite instead
of everything. Over one values.yaml per package under
packages/{apps,system,extra}, 82 of the 159 escalate on their own and
stop escalating once an unrelated suite edit joins the diff.
Coverage is now decided per changed path. Each path resolves through its
owning sources to the suites that exist, and a path reaching none of them
forces the full run whatever the rest of the diff selected. The unit is
the path rather than the source: system/postgres-operator belongs to two
PackageSources where one reaches no suite, so deciding per source would
escalate on that half and run everything for every change to it.
The escalation names its cause on stderr, as every other escalation in
this script already does. Most packages reach no suite of their own, so
this is the branch a full run usually takes; without a line saying which
sources bought it, "why did this run everything" has no answer in the log.
The walk moves into resolve_suites() to be called once per path. POSIX sh
has no `local`, so the function writes into the caller's scope and is
safe only inside the $( ) its single call site wraps it in; a test pins
that the call count stays one.
Two entries leave src_to_suites. `postgres-application` restated what
stripping the suffix produces and `external-dns` restated what carrying
the source name through produces, so both made the exception list read as
longer than the set of real exceptions.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
||
|
Aleksei Sviridkin
|
0980e755eb |
fix(ci): map E2E suites from sources not named after an app
src_to_suites() covered PackageSource names ending in -application plus external-dns, and the filter after the graph walk dropped every other source it reached. cozystack.kuberture and cozystack.securitygroup- controller are neither, so both were walked to and then discarded: a CNI change ran neither of the two controller suites most likely to regress from it, and a change to either package reached no suite at all and escalated to the full run instead of selecting the one that covers it. Every reached source now maps through the table. That table is the inverse of select-install.sh's suite_to_source(), and a test walks every suite through both, so the next source name fitting neither convention fails there rather than quietly making its own suite unreachable. The cilium test measured the gap rather than hid it, asserting the 19 suites the selector could reach and naming the two it could not. The subtraction is removed rather than inverted: kept as an exclusion list it would stay green if the walk stopped reaching them for an unrelated reason. What replaces it is a second assertion against a tree holding one suite the graph reaches and one no source names, which an escalation cannot satisfy. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
677d80a479 |
feat(kubernetes): route tenant worker image pulls through a registry mirror (#3575)
## What this PR does Refs #3513. It addresses one variant of that flake and does not close it; see "Scope against #3513" below. ### Root cause of the variant this addresses The chainsaw `kubernetes-latest` / `kubernetes-previous` suites fail with `node-join failed: fewer than 2 tenant nodes Ready within 18m` and zero registered tenant nodes. The in-guest Talos capture on #3548 shows where the time goes on one class of those runs: the worker loops trying to pull `ghcr.io/siderolabs/kubelet` from public ghcr.io and fails with `Head "https://ghcr.io/v2/siderolabs/kubelet/manifests/...": net/http: TLS handshake timeout host=ghcr.io`. The Talos `kubelet` service never starts (`kubelet was not registered`), so the node never registers, cilium-operator stays `Pending` for lack of a node to schedule on, and the budget expires. On those runs the tenant apiserver is healthy and the worker VMs boot with kube-ovn IPs, so the failing link is the worker's public image-pull egress, and the worker machine config has no `machine.registries.mirrors`, so there is no way to route around it. Same class as the Talos OS image, which #3231 solved with the `talos.imageFactoryURL` cache. The thread also carries a later analysis putting worker-VM CPU starvation from nested-virt oversubscription at the root, with serial-console RCU stalls showing the guest never reaching Talos userland, and reading the TLS-handshake variant as a symptom of the same contention. Nothing here contradicts that. Removing a live dependency on public egress from worker bring-up is worth doing on its own terms, and if the contention account is the dominant one, this narrows what is left to explain rather than competing with it. ### The change **Chart (`packages/apps/kubernetes`, `packages/apps/kubernetes-nodes`)**, a `talos.registryMirrors` passthrough, a map of upstream registry host to `{ endpoints: [ ... ] }`, rendered into `machine.registries.mirrors` of the worker `TalosConfigTemplate` that the MachineDeployment clones from, so it applies at first boot rather than after it. Empty by default: with the default values the rendered chart is byte-identical to main, both content-hash object names included, so no existing worker rolls. This is the durable half; an air-gapped, rate-limited or flaky-egress environment can point worker image pulls at its own mirror, which is the `machine.registries.mirrors` knob the Talos rollover left unfilled. Talos still falls back to the upstream registry unless a host also sets `skipFallback`, so a mirror on its own is not air-gap enforcement, and the field doc says so. The value lands inside the reconcile Job's unquoted heredoc, so it is escaped for backslash, dollar and backtick and renders as a literal. `talos.installerRepository`, `talos.schematicID` and the Talos version render into that same heredoc one block above it and now get the same treatment, so the file is consistent about it rather than escaping only the newest field. Escaping the rendered value covers every byte by construction, which enumerating known-bad shapes does not. The chain is a no-op for values carrying none of those characters, so the default render, content-hash Job name included, is unchanged and no existing worker is replaced. **e2e harness**, an in-sandbox `registry:2` pull-through cache for ghcr.io (`hack/e2e-ghcr-mirror.yaml`), applied at install time from its own step, with `hack/e2e-chainsaw/_lib/ghcr-mirror.sh` setting `spec.talos.registryMirrors` on the tenant CR when the mirror is up and its egress allow is in place, and emitting nothing otherwise so workers pull directly. A `CiliumClusterwideNetworkPolicy` opens one tightly-scoped hole for worker VM (`kubevirt.io: virt-launcher`) Pods in `tenant-test` to reach the mirror, mirroring the existing `talos-image-cache` policy. Two properties of the helper worth calling out, because both are easy to get wrong and both are now pinned by tests. A failed API call is never cached: the decision is reused by every later suite in the shared sandbox, so caching one blip would disable the mirror for the whole run. Readiness is decided once and acted on once. `rollout status` is the cheap way to learn it, but it also exits non-zero on a broken watch, so when it fails the Deployment's own `Available` condition is read rather than the watch's exit code taken as the answer. Exactly one failure is cached, a definite `Available=False`, and that is a trade rather than an invariant: it avoids re-paying the wait in every later suite, and costs a mirror that recovers mid-run. On a node-join failure the suite dumps the mirror's state alongside the Talos image cache it already dumped. Workers being pointed at the mirror and then joining does not establish that the mirror served anything, because Talos falls back to public ghcr.io on its own; the registry's access log is what separates the two, so the dump counts kubelet-image requests across the whole log rather than tailing it. The readiness probe writes an access line every five seconds, so by the time a run has spent its 18m budget a fixed tail window is hundreds of lines short of the request worth finding. ### Testing - `helm unittest packages/apps/kubernetes`, 205 tests, 22 suites; `helm unittest packages/apps/kubernetes-nodes`, 14 tests, 4 suites. New cases: the default omits `machine.registries.mirrors`, and a set value renders it with the endpoint. - `hack/ghcr-mirror_test.bats`, 23 tests. Beyond the manifest's two-phase split, egress identities, pull-through config and the pure YAML builder, this covers every outcome of `resolve_ghcr_mirror_endpoint` against a stubbed `kubectl`: an API NotFound caches, a transient query failure does not, a shell reporting a missing binary is not mistaken for either, a failed egress allow does not cache and its reason reaches the log, a rollout watch that failed for a reason the API does not confirm does not cache while a definite `Available=False` does, and a committed endpoint is cached and short-circuits the next call. The stub honours `--tail`, so the diagnostic's tests tell a filtered read from a truncated one instead of trusting the flag, and it reports errors on stderr as the real binary does. The suite also pins that the install step excludes the Cilium policy from its pre-Cilium apply, not only what that exclusion produces. - `hack/talos-reconcile-heredoc_test.bats`, 5 tests, which render both charts and run the heredoc through a real shell with hostile values rather than regexing the rendered string. A `matchRegex` cannot catch a heredoc the shell refuses to emit; this runs it. - `hack/run-kubernetes-talos-spec_test.bats`, 5 tests over `talos_spec_block`, the point where the two optional fragments are merged. The assertions splice the result under `spec:` and read it back with yq, so a block indented into the wrong parent fails instead of passing. - `packages/apps/kubernetes-nodes/tests/render-parity.sh` byte-identical; per-chart `make generate` in both charts with no drift; both `cozyrds` `openAPISchema` blocks equal to their `values.schema.json`. - The pinned `mirror.gcr.io/library/registry:2.8.3@sha256:a3d8aaa6...` is the digest Docker Hub serves for that tag, and `mirror.gcr.io` serves the same OCI index with a linux/amd64 manifest. Run under exactly the security context the Deployment sets, non-root 65532, read-only root filesystem, all capabilities dropped, writable state only on the mounted volumes, it starts, answers `/v2/` with 200, and serves `ghcr.io/siderolabs/kubelet` manifests and blobs as an anonymous pull-through. - `shellcheck` and `git diff --check` clean on the new and edited shell. ### What a CI run still has to answer Two runtime facts the harness half rests on, both recorded in the manifest: 1. Talos honours a plain-http mirror endpoint via `machine.registries.mirrors[ghcr.io].endpoints=[http://...]`. If a scheme or TLS knob is additionally needed, `machine.registries.config` has to be added too. 2. The containerd pull from inside the worker VM leaves with the virt-launcher Pod's own Cilium identity, which is what the policy selects. The label half is settled: `run-kubernetes.sh` already reads tenant worker Pods by `kubevirt.io=virt-launcher`, but whether the in-guest pull is subject to that Pod's egress policy at all is not. The second one bounds how strong a claim the harness can make. The gate is a `rollout status` plus an accepted egress-allow object, and neither observes the tenant-side datapath, so the fallback paths cannot make CI worse while the committed path is only as safe as that assumption. The in-repo comments say this rather than claiming the mirror can only ever help. ### Named improvements left out A tenant-scoped reachability probe, like the byte-level 206 check `talos-image-cache.sh` runs from a Pod carrying the consumer's own label, is what turns assumption 2 from an assertion into a measurement, and would also let the suite assert the mirror served the kubelet pull instead of the worker quietly falling back. Left out because a probe that does not exercise the tenant network path would pass while real workers stayed blocked, and the one that does is a change of its own size. The same gap has a cheaper edge: both the install step and the resolver key on the Deployment alone, so an apply that made the Deployment but not the Service still reads as success. That one costs little in practice, since the guest gets a fast NXDOMAIN instead of a dropped SYN, but the gate does check less than it commits to. `registryMirrors` could be a typed `map[string]struct{ endpoints []string }`, the way `nodeGroups` already is in this chart, so malformed input is rejected at admission instead of failing late inside Talos. The helm-unittest escape cases assert the `$` path; the backtick path is covered by the execution-level heredoc bats and the pure-backslash path by the escape chain's construction, so the security contract is fully exercised across the two layers. A backtick assertion at the helm-unittest layer would express it at the fast layer too. The escape chain is spelled out at five interpolation sites across the two reconcile-Job templates. A named helper (an `include` template) would make the escape a single reusable primitive and shrink the surface where the heredoc invariant can be forgotten; the current per-site form is correct and covered, so this is a readability refactor rather than a fix. Only `machine.registries.mirrors` is passed through. `machine.registries.config` is not, so a mirror behind a private CA or one needing credentials still has no knob, and the platform-wide `registries` value does not reach tenant workers. The chart's breaking-change note says so, and also says which knob covers which artifact: the mirror routes registry pulls, while the worker OS disk image arrives as a raw HTTP artifact under `talos.imageFactoryURL` and no registry mirror can touch it. The helper's failure model is worth revisiting as a whole rather than per call site. Every call it makes to the cluster has three outcomes (the world says no, the world says yes, the call could not be made) and a shell exit code carries two, so the third is re-derived at each site from a different signal: stderr text for a NotFound, a field value for readiness, the returned message for a failed apply. Since the resulting decision is cached for the whole sandbox, a misclassification is permanent for the run rather than a blip, which makes each of those sites load-bearing. A single prober answering `yes`/`no`/`unknown`, with the caching rule stated once over that answer, would move the question out of the branches; the current shape reaches the same behaviour but re-establishes it five times. Left out here because it is a visible rework of code this PR only extends, and it would need its own verification of every site; tracked in #3682. One nicety left in the same helper: the diagnostic reads the mirror log twice, once with `--tail=-1` to count kubelet-image requests across the whole log and once with `--tail=50 --prefix` for a bounded context tail. The second read exists for `--prefix`, which attributes each line to its pod; collapsing to one read drops that. The attribution is worthless while the mirror is a single replica and load-bearing the moment it is not, in the one artifact read after a failure, and the second read is itself wall-clock bounded, so it is left in place. ### Merge-order dependency (resolved) #3676 landed first and this branch is rebased onto it. `ghcr_mirror_diagnose` now runs in the node-join failure block under the same phase gate as its neighbours, and each of its reads is wall-clock bounded by the same `COZY_DIAG_READ_TIMEOUT`/`COZY_DIAG_READ_GRACE` the block validates, so no single read can hold the op open. It is placed after the guest captures and before the image-cache re-probe: the console evidence is irreplaceable and must not be starved of budget, and the dump is still cheaper than the Pod-creating re-probe, so the spend order puts it between the two. When the diagnostics phase runs out of budget the dump is declined out loud instead of attempted. An ungated dump would spend that time anyway and take it from the captures behind it, while the mirror's state is partly recoverable from the reads that already ran, so gating costs less than it saves. The known cost is #3686: cheap reads ahead of the gate can drain the whole budget on a viscous apiserver, and the dump is then declined in exactly the runs where the mirror is a suspect. That is a defect of the budget, not of this dump, and it is tracked there. ### Scope against #3513 #3513 asks for three things: capture the guest side, act on the evidence, and do not raise the 18m budget again. The capture landed in #3548 and this consumes its output; the budget is untouched. What this acts on is the TLS-handshake variant only. The same thread documents runs that issue no apid CSR at all inside the whole budget, time lost before Talos reaches the kubelet image, which no registry mirror can affect. So #3513 stays open after this merges, and a green suite afterwards is not evidence the flake is closed. The manifest header says so too, for whoever reads it next. ### Both tenant suites are covered `kubernetes-previous` selects the previous Kubernetes *minor* out of `packages/apps/kubernetes/files/versions.yaml`, `run_kubernetes_test 'keys | sort_by(.) | .[-2]'`, not a previously-released chart. Both suites build their tenant CR from the same in-tree chart through the same helper, so both get the mirror. ### Screenshots Not applicable; no UI changes. ### Downstream repositories Walking the trigger map in `docs/agents/contributing.md` against the diff: **cozystack/terraform-provider-cozystack is affected**. The provider is hand-written, and the map lists "add, remove or rename a field in an app's `values.schema.json`" and "change a default in an app's `values.yaml`" as triggers for its schema, model and expand/flatten pair. This adds `talos.registryMirrors` with a `{}` default to both the `kubernetes` and `kubernetes-nodes` schemas. No other repository matches a trigger. The website's managed-app reference pages regenerate from each package's `README.md` on a stable tag, and this only changes an existing package's README. I have not opened the provider follow-up and have not ticked the box, because a ticked box with no link claims work that does not exist. Leaving it to a maintainer to decide whether the provider models this field and who files it. - [ ] 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(kubernetes): add `talos.registryMirrors` to route tenant worker node image pulls through a registry mirror for air-gapped or flaky-egress environments ``` |
||
|
Aleksei Sviridkin
|
3ae1e10c92 |
docs(kubernetes): correct the registryMirrors field and README claims
State in the README that in-guest registry pulls are routable again via talos.registryMirrors, which knob covers which artifact (the mirror routes registry pulls; the worker OS disk image arrives as a raw HTTP artifact under talos.imageFactoryURL and no registry mirror touches it), and the limits: skipFallback is needed for air-gap, machine.registries.config is not passed through, and existing nodes keep their boot-time config until replaced. Trim the field doc comment. The regenerated schema, README tables and cozyrds openAPISchema follow from those sources. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> |