mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 05:25:14 +09:00
237f2e2bbd7d7d0fa7cc7eeb24542a28ec100dac
6673
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Alexey Artamonov
|
237f2e2bbd |
fix(postgres): say how to get past a refused extension clause
Before the charset guard, an extension value was spliced into CREATE EXTENSION whole, so a release can carry a clause after the name, such as earthdistance CASCADE, and that is the usual way to pull in a dependency. The guard refuses such a value on upgrade too, and keeping it that way is right, but the tenant needs to know what to write instead. The reject message now says: drop the clause, which changes nothing for an extension that is already installed because the script never drops one and CREATE EXTENSION IF NOT EXISTS leaves it alone, and list the dependencies first, since extensions are created in list order. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
ce74fdeea9 |
docs(backups): describe restored credentials and pre-upgrade residue
Say that a restored copy converges its application passwords shortly after recovery, that MariaDB backups leave out the grant table, and that dropping the password field does not rewrite copies made before the upgrade: old Backup snapshots and Helm revisions still hold the password that stays live. The credentials projector comment counts only the engines that still take a password from values. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
f02ca8cbc2 |
test(backups): log in as an application user after each restore
The backup examples and e2e suites no longer set passwords. They read the generated ones from <release>-credentials with jq, since a username may contain a dot that jsonpath treats as a path step, and log in as an application user after each restore, so a restore that leaves the credentials unusable fails the run. The MariaDB run also checks that an account created out of band on the source is absent from the copy. The unreachable-target restore step asserts a terminal Failed attributed to the recovery window, since the reason itself is best effort. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
9fd590a5a7 |
fix(backups): keep a MariaDB restore target's accounts
The operator ships mysql.global_priv with every logical dump, so a restore into a copy replaced the target's grant table with the source's password hashes: application logins failed, and root, which the operator sets only at datadir bootstrap, stayed wrong. Set ignoreGlobalPriv on every Backup the driver creates. An account created out of band on the source is therefore not carried into the copy. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
70dcd9daed |
fix(backups): let a restored Postgres app converge its credentials
A restore sets bootstrap.enabled on the target app, which skips the init-job, so the recovered roles keep the source's password hashes while the target's credentials Secret holds generated ones and every application login fails. Once recovery converges, the driver records RecoveryConverged and then clears bootstrap.enabled, so the next HelmRelease reconcile runs the init-job and applies the passwords. Ready carries RestoreCompletedCredentialsPending and an Event says the convergence is still pending. A clear that keeps failing is retried under BootstrapDisablePending within a fixed grace window after convergence and then fails the restore. bootstrap.enabled is serialized without omitempty so the patch sends false instead of dropping the key. Running the init-job on a restored cluster exposes its delete pass to a catalog the spec does not describe: the driver sets spec.databases to the Backup's snapshot, but recovery replays WAL past it, so a database created after the Backup comes back still marked as managed and would be dropped with its data. The first run after a restore, told apart by the per-RestoreJob bootstrap.newServerName that the cluster has not yet recorded, releases such a database and its roles from management instead, and the run records the value once every pass has succeeded. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
9afdc49894 |
feat(apps): warn when a removed field is still set on a database app
The apps API hands the stored spec to the HelmRelease unchanged, so a users[].password left on a Postgres or MariaDB application is silently ignored by the chart. Return a warning on create and update for each such user, and for the superuser key each chart refuses to manage, so the operator editing it learns it has no effect. Also warn when an update introduces a "_"/"-" collision between usernames, database names or Grant names, which the charts reject only on install. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
9f857f64e1 |
feat(mariadb)!: generate every user password, root included
users[].password is gone from the chart, its schema and the API type. Every password, root included, is generated into <release>-credentials and kept across reconciles by lookup, and a password left in values is ignored. An upgrade that finds no credentials Secret fails instead of minting new passwords, because mariadb-operator applies root only at datadir bootstrap and would advertise a root it never set. The Secret carries k8s.mariadb.com/watch so the operator applies a changed password without waiting for its requeue interval. A username or database name becomes a Secret key and a resource name, so render-time guards reject a name outside the DNS-1123 rule or past its limit: 80 for a username, as the User CRD allows, and 64 for a database, as the server allows. Names that differ only in "_" and "-" collide on one resource; that is rejected on install only, since an existing release already applied the overwrite. A user in both admin and readonly of a database keeps a single ALL grant under the unchanged Grant name, because renaming a Grant makes the operator revoke the live privilege. A role member past 128 characters, the server's account-name limit, and a database name of 65-80 characters, which the Database CRD accepts but the server does not, are rejected on install and on upgrade only when their Grant or Database does not exist yet, so a release that already applied one keeps upgrading. User CR names and keys are quoted so a username that reads as a YAML scalar stays a string. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Alexey Artamonov
|
2b5877eff9 |
feat(postgres)!: generate user passwords and read them at runtime
users[].password is gone from the chart, its schema and the API type. Every application-user password is generated into <release>-credentials and kept across reconciles by lookup, so an upgraded release keeps the password it already has and a password left in values is ignored. The init-job mounts that Secret and applies each password with ALTER ROLE and a psql variable, so no password is rendered into the init-script Secret that Helm keeps in release history. While bootstrap is enabled the Secret is marked credentials-pending, and NOTES say the generated passwords apply once the init-job runs. Names reach superuser SQL and the init-script shell, so render-time guards reject a username, database or extension name outside [-._a-zA-Z0-9] or past the length PostgreSQL silently truncates: 63 bytes for a role or extension, 54 for a database, whose <db>_readonly role would otherwise truncate and fail the next run. A username over 63 bytes that the credentials Secret already carries still upgrades. users.postgres, the CNPG superuser, is rejected in every bootstrap state. Changing a database's key in values drops the database, so the reject message and the README give the rename that keeps the data: a platform administrator renames it and its roles as the CNPG superuser before the key changes. The init-job cannot hang its Helm hook: the readiness wait ends on elapsed time below activeDeadlineSeconds and the Job never retries, so a purged restore target fails with a diagnostic. The delete-users pass reads managed users from pg_shdescription instead of parsing the version-dependent \du+ layout, which on PostgreSQL 16 and later matched nothing and left a removed user able to log in; it compares names as CREATE ROLE stores them and reassigns a removed user's objects in every database before dropping it. A database without roles renders, and extension names are quoted for hyphenated names and folded to lowercase so they resolve as they did unquoted. A value that carries its own surrounding quotes, the only way a hyphenated extension rendered before, has that pair removed and keeps its case, so such a release still upgrades. The README reads a password with jq, since jsonpath splits a dotted key. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io> |
||
|
Timofei Larkin
|
ce3eb30e03 |
fix(api): refuse only what a name declaration actually breaks (#4429)
## What this PR does Follow-up to #4367, taking the non-blocking findings from its approving review. Each one is a case where a name declaration (`x-cozystack-name`) was handled more harshly, or more quietly, than the declaration itself warrants. - **An unparseable schema is no longer blamed on a declaration.** A schema that is not a JSON object has no declaration in it, and the spec schema loader already reports it. The API server used to refuse every create of the kind with "declares an invalid x-cozystack-name". It now treats the schema as declaring nothing: creates keep working, and `Kubernetes` and `Kafka` keep their legacy caps. - **Annotation keywords no longer take a kind offline.** Any unknown key inside a declaration used to refuse creates, so a generator adding a `title` or an example would have done the same. JSON Schema annotation keywords (`title`, `default`, `examples`, `deprecated`, `readOnly`, `writeOnly`, `$comment`) and `x-` extensions are now ignored. A constraint keyword the server does not implement, such as `format` or a misspelled `maxLenght`, is still refused, because accepting it would drop it silently. - **The docs no longer overclaim.** They said a declaration no name can satisfy is refused. That is true only for lengths: a pattern is checked to compile, not to match anything, so `$^` rejects every name and names the pattern as the reason. The docs now say that. - **The API review gate sees the declarations the server refuses.** The gate only read a declaration that was an object, and only diffed the keywords it knew. A declaration turned into a string or null, or a misspelled keyword, looked like no change. A changed declaration now goes through the same parser the server uses, and the gate reports it if it cannot be enforced. The parser moves to `pkg/apis/apps/validation` so there is one copy of the rules. - **The declaration no longer leaks into the published OpenAPI for `spec`.** The whole `openAPISchema` is published as the schema of `spec`, so `x-cozystack-name` appeared there as a vendor extension of `spec`. It is now stripped from the published copy. `metadata.name` still carries no limit in the published OpenAPI: it is the shared `ObjectMeta` reference, and a per-kind copy is a bigger change than this one. Not changed: a pattern that can never match is still accepted. Deciding whether a regular expression matches any string within the declared lengths is a much larger piece of code than the mistake it would catch. ### How it is tested - `TestUnparseableSchemaDeclaresNothing`: three non-object schemas; creates still pass, and the Kubernetes legacy cap still holds. - `TestNameDeclarationIgnoresAnnotations`: a declaration carrying `title`, `examples`, `$comment` and an `x-` extension still enforces its `maxLength`. - `TestInvalidNameSchemaFailsClosed` gains `format` as an unsupported constraint keyword. - `TestClassifyNameDeclaration` gains a declaration turned into a string, a misspelled keyword and a null declaration, all reported, and an added annotation, which is not. - `TestPatchSpecLeavesNameDeclarationOffSpec`: the published `spec` drops `x-cozystack-name` and keeps an unrelated vendor extension. ### Downstream repositories No repository is affected beyond the follow-ups already noted on #4367 (the `cozystack/website` pages on annotations and ApplicationDefinitions). Those are still unfiled, so no box is ticked. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(api): An ApplicationDefinition whose openAPISchema cannot be parsed no longer refuses every create of its kind as an invalid name declaration, and annotation keywords (`title`, `examples`, `x-` extensions and the like) inside `x-cozystack-name` are now ignored instead of taking the kind offline. The API review gate now reports a changed name declaration that the API server would refuse, and `x-cozystack-name` no longer appears on the published OpenAPI schema of `spec`. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Application name declarations now support minimum and maximum lengths and regular-expression patterns. Matching patterns are checked for compilation; a pattern that matches no names rejects all names. * **Bug Fixes** * Invalid or unsupported name constraints now produce configuration errors and block every create of the affected kind. * Descriptive annotations and `x-` extensions are ignored when validating name declarations. * The custom name declaration is omitted from the published API specification. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
4b6933c569 |
fix(ingress): trust CF-Connecting-IP only from Cloudflare ranges (#4444)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does With `cloudflareProxy: true` on a tenant Ingress app, any client that reaches the load balancer directly can pick its own source address by sending a `CF-Connecting-IP` header, and that address is what `whitelist-source-range` checks. So the tenant `whitelist`, and any per-Ingress whitelist behind the same controller, can be bypassed with one header. The chart passes the Cloudflare ranges to the controller as `set_real_ip_from`. That is an nginx directive, not an ingress-nginx ConfigMap key, so the controller drops it without a warning. `use-forwarded-headers: "true"` turns the realip module on, and it then trusts every source from the default `proxy-real-ip-cidr`, which is `0.0.0.0/0` (ingress-nginx v1.11.5, `internal/ingress/controller/config/config.go` and the `set_real_ip_from` loop in `nginx.tmpl`). This has been the case since the option was added. The fix renames the key to `proxy-real-ip-cidr`. Traffic that comes through Cloudflare keeps the visitor address as before. A direct connection keeps its own address, whatever it puts in the header. If something between Cloudflare and the controller rewrites the source address, the header stops being trusted and the whitelist sees that hop instead; that is the correct result for a header that can only be trusted from known senders. The new test checks that the trusted ranges land under the key the controller reads, that they are not a catch-all, and that `set_real_ip_from` is gone. ### Screenshots No UI change. ### Downstream repositories Template-only change in `packages/extra/ingress`: no values key, schema, package, platform, installer or `hack/` change, so nothing in the trigger map is reached. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(ingress): with `cloudflareProxy` enabled, `CF-Connecting-IP` is now trusted only from Cloudflare's address ranges. Before, the Cloudflare ranges were passed under a key ingress-nginx ignores, so any client reaching the load balancer directly could set the header and get past the ingress `whitelist`. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated Cloudflare proxy IP configuration to use ingress-nginx’s supported setting, preserving forwarded-header and client IP handling. * Ensured Cloudflare proxy IP ranges remain configured when a whitelist is set. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
3800f63513 |
fix(ingress): trust CF-Connecting-IP only from Cloudflare ranges
cloudflareProxy passed the Cloudflare ranges to the controller under set_real_ip_from, which is an nginx directive and not an ingress-nginx ConfigMap key, so the controller dropped it without a warning. With use-forwarded-headers on, the realip module then trusted every source from the default proxy-real-ip-cidr of 0.0.0.0/0, and any client that reached the load balancer directly could set CF-Connecting-IP to an address of its choice. That address is what whitelist-source-range checks, so the tenant whitelist, and any per-Ingress one, could be walked past with one header. proxy-real-ip-cidr is the key the controller reads for the trusted ranges. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Timur Tukaev
|
5f5d8e9b7d |
docs(governance): nominate @themoriarti as Reviewer for the Proxmox integration (#4439)
## What this PR does Nominates @themoriarti (Marian Koreniuk) as a **Reviewer** for the Proxmox integration, following the process in [CONTRIBUTOR_LADDER.md](https://github.com/cozystack/cozystack/blob/main/CONTRIBUTOR_LADDER.md#reviewer): - `.github/CODEOWNERS`: adds @themoriarti as a code owner of `packages/system/capi-providers-infraprovider-proxmox/` and `packages/system/proxmox-*/` (Proxmox CCM and CSI). Each rule keeps the full `/packages/system/` owner set, so no existing owner loses coverage, and the catch-all invariant (`hack/codeowners-invariant.bats`) holds. - `MAINTAINERS.md`: adds him to the Reviewers table. The scope is deliberately narrow: shared code such as `packages/apps/kubernetes*`, `api/apps` and the generic in-cluster IPAM provider stays with the existing owners. ### Why Marian has maintained the Proxmox integration since the first Proxmox bundle and is now landing the Cluster API infrastructure provider, CCM and CSI (#4087, #4088, #4089). Against the Reviewer requirements: - well over 10 accepted PRs, contributing since 2024; - has reviewed more than 20 PRs; - in-depth knowledge of, and commitment to, the Proxmox area. **Sponsors:** @kvaps (Ænix) and @kingdonb (Urmanac), which satisfies the "at least one sponsor from a different employer" requirement. ### Notes for merging - The Proxmox package directories arrive with #4087 and #4089. Merging this PR after them is cleanest, though a rule for a path that does not exist yet is harmless. - GitHub silently ignores CODEOWNERS handles without write access. After merge, an org admin needs to add @themoriarti as a repository collaborator with the **Write** role (currently read). ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note docs(governance): add @themoriarti as Reviewer for the Proxmox integration. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Expanded code ownership and review coverage for Proxmox integrations, including infrastructure provider, cloud controller manager, and storage components. No end-user functionality changed. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
76b3652297 |
fix(kubevirt)!: evict non-migratable VMs instead of blocking the drain (#4403)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does A node drain now completes when a VM on that node cannot live-migrate. The VM is shut down and started again instead of holding the drain forever. VMs that can migrate still migrate. The change is the cluster-wide `evictionStrategy` in the KubeVirt CR, from `LiveMigrate` to `LiveMigrateIfPossible`. The drain is held by virt-api's eviction webhook, not by a PodDisruptionBudget. With `LiveMigrate` and a VMI that is not live-migratable, the webhook denies every eviction with HTTP 429 ([pod-eviction-admitter.go#L147-L150](https://github.com/kubevirt/kubevirt/blob/v1.9.0/pkg/virt-api/webhooks/validating-webhook/admitters/pod-eviction-admitter.go#L147-L150)). `kubectl drain` retries a 429 until `--timeout`, which is infinite by default. KubeVirt v1.9.0 does not create PDBs for VMIs at all; its disruption-budget controller only deletes old ones ([disruptionbudget.go#L353-L406](https://github.com/kubevirt/kubevirt/blob/v1.9.0/pkg/virt-controller/watch/drain/disruptionbudget/disruptionbudget.go#L353-L406)). With `LiveMigrateIfPossible` the same webhook lets the eviction through for such a VMI ([L152-L155](https://github.com/kubevirt/kubevirt/blob/v1.9.0/pkg/virt-api/webhooks/validating-webhook/admitters/pod-eviction-admitter.go#L152-L155)). The VM then comes back according to its `runStrategy`. RWO disks are not the only case. A VM with a passthrough GPU is not migratable either, unless the `VGPULiveMigration` gate is on and the GPU is a mediated device ([vm.go#L1310-L1319](https://github.com/kubevirt/kubevirt/blob/v1.9.0/pkg/virt-handler/vm.go#L1310-L1319)). The gate is off in Cozystack, so any `vm-instance` with `gpus` set blocked the drain of its node until now. In KubeVirt a drain and a launcher update go through the same eviction. After a KubeVirt upgrade the workload updater live-migrates outdated VMIs that can migrate and evicts the rest, because `workloadUpdateMethods` lists `Evict` as the fallback ([workload-updater.go#L421-L424](https://github.com/kubevirt/kubevirt/blob/v1.9.0/pkg/virt-controller/watch/workload-updater/workload-updater.go#L421-L424)). The eviction call ([L614-L620](https://github.com/kubevirt/kubevirt/blob/v1.9.0/pkg/virt-controller/watch/workload-updater/workload-updater.go#L614-L620)) hits the same webhook as a drain. So a `vm-instance` VM that cannot migrate either blocks both, which is what happens today, or is restarted by both, which is what happens after this PR once the VM runs under the new strategy. This has a cost. A non-migratable VM now restarts on a drain. That covers VMs on RWO storage and VMs with GPU or host device passthrough. The webhook does not look at who asked for the eviction, so this holds for any eviction through the API, not only a drain. A VM whose disk is bound to the drained node, for example on a LINSTOR class without DRBD, stays down until the node is back. The upgrade itself restarts nothing, and existing VMs switch only after their next restart. KubeVirt writes the cluster strategy into the VMI spec when the VMI is created ([defaults.go#L208-L211](https://github.com/kubevirt/kubevirt/blob/v1.9.0/pkg/defaults/defaults.go#L208-L211)), and the webhook reads that field first ([migrations.go#L116-L122](https://github.com/kubevirt/kubevirt/blob/v1.9.0/pkg/util/migrations/migrations.go#L116-L122)). A VMI that was running before the upgrade keeps `LiveMigrate`, so a drain still waits on it and the updater's eviction still gets 429. After one restart the VMI carries `LiveMigrateIfPossible`. From then on every KubeVirt bump shuts it down through the updater, in batches of 10 every minute, the `batchEvictionSize` and `batchEvictionInterval` set in the CR ([kubevirt-cr.yaml#L139-L140](https://github.com/cozystack/cozystack/blob/25d4a8c951b9fa7249b56ae79328d2cfd4f3a9c0/packages/system/kubevirt/templates/kubevirt-cr.yaml#L139-L140)). Reading the source, a CPU socket increase on such a VM under `vmRolloutStrategy: LiveUpdate` goes the same way, since the `VCPUChange` condition sends it to the updater ([L320-L325](https://github.com/kubevirt/kubevirt/blob/v1.9.0/pkg/virt-controller/watch/workload-updater/workload-updater.go#L320-L325)). I have not run that one. I kept `Evict` even so, because `kubernetes-nodes` workers depend on it. They set `evictionStrategy: External`, and for that strategy the webhook marks the VMI for evacuation instead of evicting it ([pod-eviction-admitter.go#L156-L157](https://github.com/kubevirt/kubevirt/blob/v1.9.0/pkg/virt-api/webhooks/validating-webhook/admitters/pod-eviction-admitter.go#L156-L157)). CAPK sees the mark on a VMI that cannot migrate, drains the tenant node and deletes the VMI ([machine.go#L414-L454](https://github.com/kubernetes-sigs/cluster-api-provider-kubevirt/blob/v0.1.10/pkg/kubevirt/machine.go#L414-L454)). The worker VM has `runStrategy: Always`, so KubeVirt starts it again on the new launcher. That is how a GPU worker or any other non-migratable worker gets the new launcher. Without `Evict` those workers would stay on the old launcher for good. Worker behaviour does not change with this PR, and the new test pins the method list so the fallback is not dropped later. `vm-instance` renders no `evictionStrategy` and has no value for it, so a tenant has no way to opt out of either restart. KubeVirt would honour a per-VM value over the cluster one. I filed #4419 to expose it. I picked this over the options in the issue because KubeVirt already decides migratability per VMI at eviction time. The platform CR does not need to know anything about storage, and a chart shipping a VM does not have to remember an override. Deriving the strategy from the disk access mode in a chart would also miss the GPU case. To get the new drain behaviour for a non-migratable VM that is already running, restart it once after the upgrade. Until then a drain of its node still blocks, as it does today. The comment in the CR is on the same line as the key on purpose. `make update` inserts blocks right before the `evictionStrategy:` line, so a comment above the key would get separated from it. A VM with `runStrategy: RerunOnFailure`, `Manual` or `Once` may stay off after an eviction. If the guest shuts down cleanly on the ACPI request within its grace period, the VMI ends `Succeeded`. `RerunOnFailure` restarts only a failed VMI, and `Manual` and `Once` restart nothing on their own ([vm.go#L1039-L1149](https://github.com/kubevirt/kubevirt/blob/v1.9.0/pkg/virt-controller/watch/vm/vm.go#L1039-L1149)). `Once` stays off even after an unclean stop. Only the default `Always` brings the VM back in every case. What I did not verify. I have not drained a node with such a VM on a live cluster, and I have not watched the updater restart them during an upgrade. Everything above comes from the KubeVirt v1.9.0 and CAPK v0.1.10 source. I did not check how `talosctl upgrade` handles a drain that keeps getting 429. I also did not check how the cluster autoscaler treats a node with a non-migratable VM now that its launcher pod can be evicted. The pod has no `safe-to-evict` annotation, so for a VM on node-local storage that likely comes down to the autoscaler's `skip-nodes-with-local-storage` setting. Finally, I did not check whether the upstream `VMCannotBeEvicted` alert is deployed in Cozystack. For non-migratable VMs it stops firing after this change, since they are evictable now. Closes #4360 ### 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. --> 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: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(kubevirt)!: VMs that cannot live-migrate no longer block a node drain. The cluster-wide eviction strategy is now LiveMigrateIfPossible instead of LiveMigrate. VMs that can migrate still migrate. VMs that cannot, for example with a ReadWriteOnce disk or GPU or host device passthrough, are shut down and started again by their runStrategy. This applies to any pod eviction, not only a drain. A VM whose disk is bound to the drained node stays down until the node returns. A VM that is already running keeps the old strategy until its next restart, so restart non-migratable VMs once after upgrading if their nodes need to drain. The KubeVirt workload update uses the same eviction: once a non-migratable vm-instance VM runs under the new strategy, every later KubeVirt upgrade shuts it down, 10 VMs per minute. A VM with runStrategy RerunOnFailure, Manual or Once that shuts down cleanly on eviction stays off until it is started again. kubernetes-nodes workers are not affected. ``` |
||
|
tym83
|
e24ffa3bce |
docs(governance): nominate @themoriarti as Reviewer for the Proxmox integration
Marian has driven the Proxmox integration since the first Proxmox bundle and is now landing the Cluster API infrastructure provider, CCM and CSI. Giving him code-owner approval on those packages lets Proxmox changes be reviewed by the person who knows them best, without granting approval rights over the rest of the tree. Assisted-by: LLM Signed-off-by: tym83 <6355522@gmail.com> |
||
|
Aleksei Sviridkin
|
c1c50e50f6 |
fix(kafka): use the shared wait_hr_ready body in the topic-data example (#4436)
## What this PR does [#3580](https://github.com/cozystack/cozystack/pull/3580) landed with the old `wait_hr_ready` in `examples/backups/kafka/00-helpers.sh`. It was branched before [#3769](https://github.com/cozystack/cozystack/pull/3769) moved the backup examples to one body, and `hack/mariadb-first-boot-waits.bats` runs every copy it finds, so unit tests are red on main now and on every PR tested against it ([example run](https://github.com/cozystack/cozystack/actions/runs/35978978386/job/107566130505)). Function is replaced with the mariadb copy, rest of the file untouched. `make unit-tests` passes locally. ### Screenshots ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(kafka): the topic-data backup example now reports why a HelmRelease wait failed, same as the other backup examples. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved diagnostics when a Kafka backup release fails to become ready. Messages now distinguish lookup failures, releases that never appear, and releases deleted while waiting. * Failure reports include release conditions and available deployment history, making it easier to understand stalled or timed-out releases. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
08c535c295 |
fix(monitoring): give kube-apiserver its own scrape size limit (#4427)
## What this PR does kube-apiserver's `/metrics` grows with the number of resources and CRDs, and on a full install it outgrows the 32MB `promscrape.maxScrapeSize` that monitoring-agents sets globally. vmagent then refuses every scrape of the API server, `up` drops to 0, and `KubeAPIDown` and `TargetDown` fire against a healthy control plane while every rule over API server metrics goes blind. The apiserver `VMServiceScrape` endpoint now sets its own `max_scrape_size` (128MB by default, `vmagent.apiserverMaxScrapeSize`). vmagent uses a per-job value in place of the global flag for that job only, so the 32MB guard for every other target is unchanged. The value stays finite on purpose: vmagent reads the whole uncompressed response into memory before parsing, `streamParse` included, so this limit is also a memory ceiling. A new `VMAgentScrapeSizeExceeded` warning fires on `vm_promscrape_max_scrape_size_exceeded_errors_total`, so an oversized target is reported as a size problem instead of only as a target that is down. The counter has no target label, so the description points at the vmagent targets page to find it. Upgrade note: the per-job value replaces the global flag in both directions. An install that raised `promscrape.maxScrapeSize` above 128MB through `vmagent.extraArgs` should raise `vmagent.apiserverMaxScrapeSize` as well. Verified on a dev cluster with the same operator (v0.68.4) and vmagent (v1.136.0): a test vmagent with a 1MB global limit scraped all three API servers with `max_scrape_size: 64MB` on the endpoint and refused all three without it. The new counter reached storage with the `namespace` and `pod` labels the alert uses, and the alert expression returned a series. The `VMRule` passes the operator's admission webhook, which rejects a malformed expression. Guest clusters get the same default, since their HelmRelease overrides other `vmagent` keys only. Fixes #4343 ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(monitoring): kube-apiserver is scraped with its own 128MB size limit (`vmagent.apiserverMaxScrapeSize` in the monitoring-agents component) instead of the global 32MB, so a large `/metrics` no longer makes KubeAPIDown fire on a healthy API server. A new VMAgentScrapeSizeExceeded alert reports any target whose response exceeds the scrape size limit. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Kubernetes API server metrics can now use a configurable scrape-size limit, set to 128 MB by default. Leaving the setting empty uses the global limit. * **Bug Fixes** * Added a warning alert when vmagent reports scrape-size-limit errors. The alert identifies the affected vmagent pod and service and provides guidance for finding the target and adjusting its limit. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
15035a915b |
fix(vm-instance): disable helm readiness wait on the VMInstance release (#3735)
## What this PR does A VMInstance is served as a Flux HelmRelease that renders a KubeVirt VirtualMachine. Since v1.5.0 (helm-controller v1.4.3 to v1.5.0) that release is health-assessed by the kstatus poller. vm-instance sets no `healthCheckExprs`, so the VirtualMachine goes through kstatus's own rules in `fluxcd/cli-utils` (`pkg/kstatus/status`): `checkGeneration` returns InProgress for any object whose `status.observedGeneration` differs from `metadata.generation`, and past that check a `Ready=False` condition is InProgress too, so a halted VM is never Current even with its generation in sync. KubeVirt keeps a VirtualMachine's `observedGeneration` behind its `generation` whenever a `spec.template` change is pending a restart, the VM is stopped, or a live-updatable change never converges (kubevirt/kubevirt#18700). So a healthy, Running VM is read as permanently InProgress, the upgrade waits its full timeout, and fails with `timeout waiting for: [VirtualMachine/... status: 'InProgress']`. On releases still carrying the pre-1.5.0 `remediation.retries: -1` + rollback spec, the timeout then reverts the previous manifest, dropping just-applied changes such as opened ports. This sets `release.cozystack.io/helm-install-disable-wait: "true"` on the vm-instance ApplicationDefinition, so cozystack-api generates the HelmRelease with Install/Upgrade DisableWait. The release settles once the VirtualMachine is applied, and VM health is read off the VirtualMachine itself (`status.printableStatus` / `Ready`) and off the WorkloadMonitor the chart already renders, which is what `WorkloadsReady` on the Application reports. This restores the pre-1.5.0 behaviour, under which the VM was never a helm readiness gate. The same mechanism is already used by `kubernetes-rd` and `kubernetes-nodes-rd`. On its own that annotation does not reach a running cluster, so two gaps in cozystack-controller are closed with it. The first is the rollout. `applicationdefinition-controller` hashed each definition's name and spec only, so a change confined to a `release.cozystack.io/*` annotation left the hash unmoved and cozystack-api kept serving the configuration it read at its last start-up, which is the only place those annotations are ever read. The hash now covers them too. Only that prefix, because kubectl and Helm rewrite annotations of their own on every apply and hashing those would roll the api Deployment on every reconcile. The second is the releases that already exist. `applicationdefinition-helm-reconciler` brings chartRef, valuesFrom and labels on existing HelmReleases in line with their definition; it now does the same for the readiness wait. Without it the annotation reaches new instances only, which on an affected cluster is the wrong half: #3734 reports 134 of 136 releases already looping upgrade and timeout, and re-applying every VMInstance by hand was the only way out. The setting is driven in both directions, since cozystack-api rebuilds the whole spec on every write and emits `DisableWait=false` once the annotation is gone. An absent install or upgrade block already behaves as `DisableWait=false` and is materialised only when the definition asks to disable the wait, so a release with nothing to change is not rewritten. Two alternatives were considered. A per-kind `healthCheckExprs` keyed on `status.printableStatus` cannot work: setting it moves the VirtualMachine from kstatus to the CEL status evaluator (`fluxcd/pkg/runtime/cel`), which runs the same `observedGeneration` check ahead of the expressions, and any `healthCheckExprs` added to this kind later is a no-op while the wait is disabled. The typed `spec.release.waitStrategy: legacy` is the narrower option, since the legacy checker never waits on a custom resource, but it still gates the release on the chart's Service, and with `external: true` that is a LoadBalancer the legacy checker holds not ready until it has ingress. So an external VM would still wait on IP allocation, which `WorkloadsReady` already watches. Disable-wait avoids that and is the mechanism this repository already uses for the same class of problem. Tests: a helm-unittest suite (`tests/readiness_gate_test.yaml`) guards that the annotation stays on the rendered ApplicationDefinition, and Go unit tests cover both cozystack-controller paths: the hash moves on a release annotation and stands still on an unrelated one, and the wait lands on an existing release, is withdrawn when the annotation goes, does not materialise empty install/upgrade blocks, does not rewrite a release already in sync, leaves a release alone when the annotation does not parse, and does not reach a release of another kind. Note on existing clusters: no manual step. The definition changes on upgrade, the config-hash rolls cozystack-api, and the helm reconciler brings existing VMInstance releases in line. Releases that predate v1.5.0 also carry a `remediation.retries: -1` + rollback spec, which is left alone: with the wait disabled the upgrade no longer times out, so that remediation no longer fires, and any write through the apps API drops it. Closes #3734. ### Downstream repositories - [x] No downstream repository is affected by this change <!-- Walked the trigger map against the diff. The change is a readiness annotation on an ApplicationDefinition plus two cozystack-controller paths. It does not touch openAPISchema, kind, plural, release.prefix, any app package, any values.schema.json or any installer/platform values key. The website's application-definitions.md describes the config-hash as a checksum "over the sorted definitions", which stays accurate now that the hash also covers the release.cozystack.io/* annotations, and no page documents those annotations today. --> ### Release note ```release-note fix(vm-instance): VMInstance HelmReleases no longer hang InProgress or roll back healthy VMs; the release no longer waits on VirtualMachine readiness, which helm-controller's kstatus poller cannot assess correctly for KubeVirt VMs. Existing VMInstance releases are brought in line automatically on upgrade. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * VM installations no longer wait indefinitely when Kubernetes reports the VM’s observed generation behind its current generation. * Changes to release settings are now detected and applied to generated Helm releases, including install and upgrade wait behavior. * **Tests** * Added coverage for VM readiness and release-setting reconciliation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Myasnikov Daniil
|
cadd72a4be |
fix(kafka): use the shared wait_hr_ready body in the topic-data example
The topic-data example was branched before the backup examples moved to one wait_hr_ready body and merged after they did, so it carried the old copy. hack/mariadb-first-boot-waits.bats finds every copy in the tree and runs its failure branches under errexit, and the old one does not report a kubectl that cannot answer. That turned the unit job red on main and on every pull request tested against it. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com> |
||
|
Timofei Larkin
|
95ece888ba |
feat(kafka): add topic-data backup/restore example and e2e roundtrip (#3580)
## What this PR does
Adds a worked backup/restore example for a Cozystack-managed `Kafka`
application's **topic data**, built on the generic `Job` backup strategy
— the same pattern as `examples/backups/nats`, with no purpose-built
backup image. A stock Strimzi Kafka image (the `kafka-*.sh` CLI plus
`bash`, `curl` and `tar`) runs one shell script that branches on `{{
.Mode }}`: on backup it freezes each partition's end offset and drains
`[begin, end)` to a tarball it `PUT`s to S3; on restore it `GET`s the
tarball, recreates the topics with their original partition count and
replication factor, and replays every partition file. S3 access is `curl
--aws-sigv4`, verified against the endpoint's CA projected from the
`<app>-backup-s3` Secret, so no extra client image is needed.
The consistency model is a frozen-end-offset cut: `kafka-get-offsets
--time -1` is read once per partition at backup start and the consumer
drains only up to that offset, so anything produced during the run is
excluded and every partition of a topic is captured as of the same
instant — a cut by log position, independent of record timestamps.
This is deliberately a DATA-only backup. Topic configs, ACLs, SCRAM
users and **consumer-group offsets** are not captured, and re-produced
records receive fresh offsets — the README spells out the scope and
points at a volume-snapshot strategy for a faithful, offset-preserving
backup. Keyed records reproduce into their original partition because
the topic is recreated with the same partition count and the default
(murmur2) partitioner is deterministic.
The script fails closed rather than reporting a partial backup or a
lossy restore as success: it refuses an offset listing short of the
topic's partition count, a drain short of `end - begin`, a `topics`
value with a duplicate or a name outside Kafka's charset, a manifest
that disagrees with the data files, a target topic that already holds
records (checked before the replay, so a retried Pod cannot append a
second copy), a pre-existing topic with a different partition count or
replication factor, a replication factor below the target's effective
`min.insync.replicas`, and a post-replay count that does not match the
manifest. Each topic's replication factor is recorded in the manifest
and restored as captured; the `replicationFactor` BackupClass parameter
overrides it for restoring into a smaller cluster.
Also adds a `kafka-2-backup-roundtrip` Chainsaw test to
`hack/e2e-chainsaw/kafka`, which drives the example `run-all.sh` as its
harness (Bucket → two-broker source Kafka with a seeded 3-partition,
2-replica topic plus a decoy topic that the demo topic matches as a
regex but not as a literal → BackupJob → in-place restore with count,
content and replication-factor assertions → a second RestoreJob into the
populated topic that must be refused with the topic untouched →
two-broker target Kafka + to-copy restore + verify), mirroring
`mariadb-2-backup-roundtrip`. Two brokers, not one, because that is the
shape on which the chart sets `min.insync.replicas: 2` and a restore
with the wrong replication factor cannot land a record. It resolves the
Kafka image from the running operator so the backup Pod reuses the image
already cached on the nodes.
The strategy script itself is pinned by
`hack/kafka-topic-backup-strategy.bats`, on the shape of
`hack/kafka-backup-script.bats`: it renders the strategy through a stub
`kubectl` exactly as step 01 applies it, lifts the container body out,
and executes it against stubbed `kafka-*.sh`, a stubbed `curl` and a
directory standing in for S3, with fault injection for the broker
behaviours the guards exist for. Fourteen cases, one per guard; removing
the partition-listing guard reddens one of them.
Verified: `chainsaw/kafka-2-backup-roundtrip` passed in this PR's CI on
the two-broker shape (record count, byte-identical content dump and
replication factor 2 asserted after both restores, the refused
RestoreJob settled Failed with the topic untouched);
`hack/kafka-topic-backup-strategy.bats` passes under both
`hack/cozytest.sh` and bats.
Files:
- `examples/backups/kafka/` — 14 files (`00-helpers.sh`, `01`..`07`,
`run-all.sh`, `cleanup.sh`, `README.md`, `90`/`91`/`92` scenario
walk-throughs).
- `hack/kafka-topic-backup-strategy.bats` — hermetic test of the
strategy script.
- `hack/e2e-chainsaw/kafka/chainsaw-test.yaml` — new
`kafka-2-backup-roundtrip` Test appended.
- `hack/backup-access-preflight.bats` — the kafka example and Test join
the S3-preflight roster.
- `hack/select-e2e.sh`, `hack/e2e-chainsaw/.chainsaw.yaml` —
comment-only: their enumerations of preflight-gated suites are replaced
by a pointer.
- `.gitignore` — the example's `.source-dump.txt` snapshot.
No package sources (`values.yaml`, `values.schema.json`, `Chart.yaml`,
package `README.md`) are touched, so `make generate` produces no diff.
### Screenshots
N/A — no UI changes.
### Downstream repositories
Walked the trigger map in `docs/agents/contributing.md` file-by-file
against the diff. The change adds example scripts under
`examples/backups/kafka`, one unit test under `hack/`, and appends one
Chainsaw Test to an existing file under `hack/e2e-chainsaw/kafka` — it
adds no package under `packages/apps` or `packages/extra`, changes no
CRD, no `ApplicationDefinition`, no `values.schema.json`, no
platform/installer values, no `hack/*.mk` anchor or make-target
behaviour, and no node prerequisites in `hack/e2e-prepare-cluster.bats`.
None of the downstream triggers match.
- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
### Release note
```release-note
feat(kafka): add a topic-data backup/restore example and e2e round-trip on the generic Job backup strategy (data-only: topic records with a frozen-end-offset consistency cut and partition-preserving replay; consumer offsets, topic configs, ACLs and users are out of scope)
```
|
||
|
Andrey Kolkov
|
61a7a257a1 |
fix(backups): fail Velero RestoreJob rename for non-VMInstance kinds (#4426)
## What this PR does Fixes #3537. A `RestoreJob` with `spec.targetApplicationRef.name` set to a new name and `options.targetNamespace` set to another namespace reported `phase: Succeeded` for every Velero-backed application, but only a `VMInstance` was actually renamed. `postRestoreRename` looks up the restored HelmRelease under the `vm-instance-` prefix, gets `NotFound` for any other kind and returns `nil`, so a Postgres, MariaDB, ClickHouse, MongoDB or standalone VMDisk copy came up under the **source** name and nothing was surfaced. Renaming those kinds through the Velero strategy cannot work by construction: Velero DataUpload restores volumes under their original names, and only a `VMInstance` keeps using them after its HelmRelease is renamed (its disks keep their names and CDI adopts the restored PVCs). A renamed database `Cluster` or `VMDisk` renders fresh volumes for the new name and comes up empty next to orphaned source volumes. This PR takes the short-term, correctness half of the issue: the rename is rejected up front for every kind other than `VMInstance`, before a new RestoreJob enters `Running` or a Velero `Restore` is created, with a `Ready=False` message that names the kind and points at omitting `targetApplicationRef.name` or, for kinds that have one, using the application's engine-native strategy (for Postgres that is the CNPG strategy, which supports restore-to-copy). The existing same-namespace guard keeps precedence, so its message is unchanged. A rename RestoreJob that is already `Running` when a controller with this check starts is failed on its next reconcile, after its Velero `Restore` may have completed. Such a job says in its message that the application may already sit in the target namespace under its source name (a retry without the rename would otherwise trip `failIfTargetExists` on that copy), and its resource-modifier ConfigMap is released immediately rather than at the job's deletion. Jobs already `Succeeded` or `Failed` are never revisited. Rename-on-restore for non-VM applications through engine-native recovery (the longer-term feature in the issue) is out of scope here. A standalone `VMDisk` is rejected as well: its rename needs a post-restore PVC rebind and DataVolume replacement that do not exist yet, tracked in #4428. Tests: `TestVeleroRenameSupported` and `TestReconcileVeleroRestore_RenameGuard`. Rejected inputs: Postgres and VMDisk cross-namespace rename, and a VMDisk backup restored with a `VMInstance` target kind (the guard keys on the backup's kind, since that is what `postRestoreRename` acts on) → `Failed` with the new message; Postgres same-namespace rename → the pre-existing DataUpload message. Accepted inputs: VMInstance cross-namespace rename, a VMDisk copy that keeps its name, and a VMDisk in-place restore without `targetApplicationRef` → `Running`. The rejected cases fail on `main` (phase `Running` instead of `Failed`); the accepted cases fail if the `IsRenamed` conjunct is dropped. `TestReconcileVeleroRestore_RenameGuard_InFlight` covers the already-running case for VMDisk (`Failed`, message names the source-name copy, ConfigMap gone) and VMInstance (`Succeeded`). ### Screenshots N/A — no UI changes. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff: two Go files under `internal/backupcontroller/`, no API, CRD, chart, values, schema or docs change. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(backups): a Velero-strategy RestoreJob that requests a different targetApplicationRef.name for an application kind other than VMInstance (Postgres, MariaDB, ClickHouse, MongoDB, VMDisk) now fails with an explicit message instead of reporting Succeeded with the application restored under its original name. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Velero restores now fail with a clear message when a requested application rename isn’t supported for that application type. * If an unsupported rename is detected after a restore has started, the restore is stopped and its temporary configuration is cleaned up. The message notes that resources may already have been restored under the original name. * Renames for VM instances and restores that keep the original application name continue to work. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
e9868b4f62 |
fix(api): keep a suspended HelmRelease suspended across an app update (#4411)
## What this PR does This PR makes `REST.Update` keep live `spec.suspend` of the HelmRelease, on first write and on conflict retry. Update rebuilds HelmRelease from Application and writes it back whole, Application has no suspension field, so every edit through aggregated API resumed a release that operator or controller had suspended. Caller that depends on it is cnpg restore driver. It suspends target release, patches Postgres app through this API, purges Cluster and resumes only after purge is done, so next chart render lands `bootstrap.recovery` on empty namespace. When the patch resumes release, helm-controller upgrades right away, chart can render recovery Cluster while purge is still running, then driver deletes it as the Cluster being replaced and nothing renders it again, because values do not change anymore. RestoreJob stays `Running` until its deadline. On main it works only because of timing. Same write also drops `finalizers.fluxcd.io`, and helm-controller v1.5.0 spends its first pass after that on putting finalizer back and requeue (750ms minimum retry delay) before it upgrades, this delay usually lets purge finish first. #3426 keeps controller finalizers across this write, so delay is gone, and `postgres-2-backup-roundtrip` failed on both its runs there while it passes on other PRs. In [35714873620](https://github.com/cozystack/cozystack/actions/runs/35714873620) stuck restore is `pg-src-to-pg-target-pitr`: `startedAt` 11:40:37, helm upgrade v3 at 11:40:37, new Cluster created at 11:40:38 and gone by 11:40:47, no render after that. [35652855170](https://github.com/cozystack/cozystack/actions/runs/35652855170) has same picture on first restore, all inside 22:10:51. Tests are in `rest_suspend_test.go`: edit leaves `spec.suspend` as it was (both values) with values still taken from Application, and suspend that lands between read and write survives conflict retry. Removing either line fails its test. ### Downstream repositories Change is in how aggregated API writes HelmRelease back, no downstream repo restates that. Rollback guide on website (`guides/concepts.md`) tells operators to suspend HelmRelease while they work, with this fix that suspension holds across app edit, so docs need no change. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(api): app edit through cozystack API no longer resumes a suspended HelmRelease, so postgres restore waits for its purge before cluster is rendered again. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Application updates now preserve a Helm release’s suspended or active state instead of unintentionally changing it. * The release state is also preserved when an update encounters a conflict and retries, preventing a concurrent suspension from being lost. * Application updates continue to use the application’s configured values while retaining the release’s current suspension state. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
myasnikovdaniil
|
9bec21461d |
feat(kubernetes): boot tenant worker disks from a shared golden Talos image (CDI clone) (#4171)
## What this PR does Tenant worker boots from talos OS disk image that CDI streams over HTTP from image factory, and it does that once per worker. Same bytes N times, and N chances for a fetch to fail. In closed or rate-limited environment that is wrong shape. This PR adds opt-in catalog and per-pool selector that reads from it. `packages/system/kubernetes-worker-image` imports one golden DataVolume into `cozy-public` per `(schematicID, version)`, named `talos-worker-<schematicID>-<version>`. Pool opts in with `osImage.builtin` on its own `KubernetesNodes`, and its `disk-system` DataVolume then carries `source.pvc` pointing at that golden, so CDI does storage layer CSI clone instead of HTTP import. One import per flavor, shared by every worker of every tenant. Catalog is disabled by default, enable by adding `cozystack.kubernetes-worker-image` to `bundles.enabledPackages`. Its knobs (`imageFactoryURL`, `storageClass`, `storage`, `images`) are set through `spec.components.platform.values.kubernetesWorkerImage` on cozystack-platform Package. That needed optional package helper to learn components argument, which `cozystack.platform.package.optional.default` did not have, so every optional package registered through that wrapper could name variant and nothing else. Thirteen other packages get same path for free. `osImage` has two mutually exclusive forms: - `osImage.builtin` boots pool workers by CDI-cloning golden from catalog. Clone is storage layer CSI copy, so OS image never crosses pod network and factory answers once for whole fleet instead of once per worker. - `osImage.factory` keeps HTTP import and lets pool pin own `imageFactoryURL`, `schematicID` and `version`. This is bring-your-own path for operator running own factory or caching mirror, and it is what pool gets when `osImage` is omitted. In-guest talos installer written by reconcile job follows same resolution, so pool that pins release boots it and upgrades into it instead of being flipped back to cluster default halfway. This is pool half of what #3950 asks for. That issue wants platform level defaults for two channels, worker OS image and in-guest registry mirrors. Here image channel gets per-pool source and shared cache, platform default plumbing is left to the issue. ## Existing workers do not roll With `osImage` omitted rendered `KubevirtMachineTemplate` is byte identical to what chart produced before. KMT is named by hash of its own content, so any drift would re-image every worker VM in fleet on next upgrade. Stored render snapshot is untouched by this PR, that is the evidence and not the claim. Editing `osImage` on existing pool re-images that pool, like changing any other image reference. ## What fails at render Clone that would silently degrade into something slower or wrong is refused instead of rendered. Golden absent, or its import in terminal `Failed` phase. Pool `storageClass` different from golden one, because CDI cant CSI-clone across storage classes and falls back to host assisted copy over pod network, which is exactly the transfer `osImage.builtin` removes. Empty class on either side resolves against cluster default class before comparing, so pool at `storageClass: ""` is checked too. `diskSize` below golden storage, because clone target cannot be smaller than source. Talos release the matrix lists as unsupported, whether pool reached it through `talos.version` or through `osImage.*` override. Matrix passes talos minor it does not list, so what it promises is that known-bad pairing is refused, not that every pairing is known. `schematicID` that is not plain lowercase alphanumeric, or version that is not `vMAJOR.MINOR.PATCH`, both are interpolated into DataVolume name and image URL while `values.schema.json` types them as bare strings. Golden that is only mid-import is not an error. CDI holds worker disk until source populates, and failing there would abort whole pool HelmRelease for length of a download. Same reasoning covers pool that already cloned: absent or `Failed` golden fails render only for pool still waiting for its first disk, otherwise reconcile of healthy pool would abort over source it no longer reads. Golden that declares no StorageClass is read from its PVC, not from whatever default is in force today, because those differ once cluster default moves. Cluster with two default classes resolves same way Kubernetes does, newest wins. Pool that already cloned is identified by annotation naming exactly that pool, not by disk name prefix, because pool `md0-gpu` produces disks carrying `md0` prefix, and it reads its own class from its own bound PVC, so admin moving `storageclass.kubernetes.io/is-default-class` does not fail release of pool whose disks are all on right class. That resolved class is then written onto worker disk. Reading one class and binding another is how pool at empty `storageClass` passed the comparison and still took host assisted copy on its next worker, so guard and disk now come from one value. HTTP path leaves empty class to cluster as before, which is what keeps its render byte identical. Catalog side refuses duplicate `(schematicID, version)` entries, entries whose schematic or version cannot produce name pool can reconstruct, and edit to `storage`, `storageClass` or source URL of entry whose golden already exists. Last one is not patchable in place, so without guard it lands as rejected patch on next upgrade. Goldens carry `helm.sh/resource-policy: keep`, so dropping entry stops managing golden instead of deleting it. Missing golden aborts whole Kubernetes HelmRelease of every tenant pinned to it, not just the disk, and each cloned worker disk annotates golden it came from, so "does anything still clone this" is answerable before manual delete. Three drift guards live in `hack/check-worker-image-catalog-defaults.bats`, because values that have to agree sit in files nobody edits together. One-sided talos bump trips all three. ## Upgrade Three pool values that never produced working image rendered before and hard-fail now, so pool carrying any of them flips its HelmRelease to `InstallFailed` on upgrade and not on next edit: `talos.version` in `vMAJOR.MINOR` form, `schematicID` that is not plain lowercase alphanumeric, and empty `talos.imageFactoryURL`, which used to render schemeless `/image/...` source URL. None of the three could boot worker, so this is failing earlier and not a regression, but it surfaces on pool nobody touched and fix in each case is to correct the value. CR carrying such value is admitted by API with value intact, and later unrelated edit reaches `.spec.values` of HelmRelease while it stays `InstallFailed`, so edit is taken everywhere and applied nowhere. ## Why e2e does not exercise the clone path Clone is storage layer copy, so it needs StorageClass whose volumes are reachable from any node. Merge-gating e2e has none: those lanes run on talos containers, which share runner kernel and cannot load DRBD, so node-local `local` is only class there. Nothing would arrange the co-location such a class requires either, golden in `cozy-public` has no consumer and binds where CDI importer landed, while worker disk binds where its VM is scheduled. So the suites keep importing over HTTP with `osImage` omitted, which is also the render the no-roll guarantee rests on. This PR touches no e2e harness file: `hack/` differs from main only by the two guards this feature owns, one tying catalog default golden to pool chart talos and StorageClass defaults, one on in-guest installer heredoc. Switching suites over does not work on that substrate. Golden asks for `replicated`, lane does not have it, CDI resolves no StorageProfile, and catalog HelmRelease times out on DataVolume stuck in `ImportInProgress`. Passing lane own class would fix the class and leave the co-location. Nightly and tag build run same suites on QEMU with DRBD and `replicated`, which is the substrate clone path was verified on. That is where e2e coverage belongs, as its own change with that lane storage headroom measured rather than assumed. Worth being straight about what this leaves, because it bears on whether the feature earns its place. The e2e argument was never the strong one: golden fetches from image factory exactly once, and #3513 established that node-join stall tracks host `kvm_amd` `vgif` flag rather than registry egress. What clone genuinely removes is the fanout, N workers streaming over pod network becomes N storage layer copies. That is property of storage layer, so it is worth having where DRBD is and worth nothing where it is not. This ships as production capability with the constraint stated on the field, not as CI improvement. ## Testing On this tree: - full `make unit-tests` plus `make test-controllers`: 95 charts, 2520 helm-unittest cases, 1710 bats cases, `go test ./pkg/...` and `./internal/...`, exit 0 - `pre-commit run --all-files`: exit 0, root `make generate` and per-app generate both clean, so no generated-file drift - render snapshot unchanged, which is what pins byte identical render for pool that does not adopt `osImage` - mutation pass over render guards, each term deleted or inverted in turn and suite required to notice it by name. Restoring presence gate on catalog StorageClass comparison reddens exactly one case, inverting its `ne` to `eq` reddens five. Reverting worker disk to omit its resolved class reddens three cases that pin where builtin disk binds, and forcing that class non-empty on every path reddens HTTP-path case and all three render snapshots, with snapshot diff naming the changed KMT hash `E2E Tests` passed on run 35831830568. That run was on the previous head, 4fd8e7ba, and this head differs from it only by one test comment and the commit message. ## Verified on a live cluster Throwaway three-node talos stand (OCI, talos v1.13.6, LINSTOR/ZFS, `replicated` StorageClass) installed from this branch `packages/` tree. This covers what neither `helm template` nor unit suites can answer. It predates the change that writes resolved StorageClass onto worker disk, and that stand ran with explicit `replicated` on both sides, which is the value that change now renders. - The catalog wires up from `bundles.enabledPackages` alone: `PackageSource cozystack.kubernetes-worker-image` reconciled, its HelmRelease installed, and the golden `talos-worker-<schematic>-v1.13.6` imported to `Succeeded` in 108s (6Gi, RWX, `replicated`). - A pool on `osImage.builtin` clones it on the storage layer. Both worker DataVolumes went `CSICloneInProgress` then `Succeeded` in 43s with `Running=False reason=Completed message="Clone Complete"`, carrying `source.pvc` and no `source.http`, and no importer or upload Pod appeared in the tenant namespace. - The clone yields a bootable talos root disk. Both worker VMs reached `Running`/`Ready`, joined the tenant cluster and went `Ready` about 66s after creation, and the tenant cilium, coredns, csi, ingress-nginx and metrics-server addons all installed on them. - Access modes match across the hop: golden `ReadWriteMany` 6Gi to worker `ReadWriteMany` 20Gi on the same `replicated` class, grown by CDI. - The written annotations are correct in practice: each worker disk carries `kubernetes-worker-image.cozystack.io/golden` naming the golden and `.../pool` naming its pool. - helm-controller converges on a builtin pool: forcing a reconcile left the content-hashed KMT name unchanged and the VMI and DataVolume UIDs identical, so a reconcile does not roll the pool. - Cross-namespace clone RBAC is in place as shipped: `create datavolumes --subresource=source -n cozy-public` resolves to `yes` for a tenant ServiceAccount through the `cdi-clone-dv` RoleBinding, and CDI runs with `cloneStrategyOverride: csi-clone`. This PR adds no RBAC of its own. ### Screenshots Not applicable, no UI change. ### Downstream repositories Trigger map points at two repositories. Provider models every pool field by hand, so `osImage` needs schema, model and expand/flatten pair there. Already tracked as cozystack/terraform-provider-cozystack#39, filed when the field was still called `image`. `content/en/docs/next/operations/configuration/platform-package.md` is a hand-written table of `spec.components.platform.values.*` keys and nothing generates it, so `kubernetesWorkerImage` needs a row there. Filed as cozystack/website#707. - [ ] No downstream repository is affected by this change - [x] [cozystack/website](https://github.com/cozystack/website) - follow-up: https://github.com/cozystack/website/issues/707 - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: https://github.com/cozystack/terraform-provider-cozystack/issues/39 - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note feat(kubernetes): tenant worker pools can boot their system disk from a shared golden talos image instead of importing it from the Image Factory once per worker. Enable the opt-in `cozystack.kubernetes-worker-image` package via `bundles.enabledPackages` to publish one golden DataVolume per `(schematicID, version)` in `cozy-public`, then set `osImage.builtin` on a `KubernetesNodes` pool to have CDI clone it on the storage layer. A pool that omits `osImage` keeps importing over HTTP and renders exactly as before. The clone path needs the pool and the golden on the same StorageClass, and one whose volumes are reachable from any node, in practice a replicated/DRBD class. ``` |
||
|
Timofei Larkin
|
a461dc4b38 |
fix(api): keep the name declaration off the published spec schema
The whole openAPISchema is published as the schema of spec, so x-cozystack-name showed up as a vendor extension of spec in the aggregated OpenAPI. That describes something spec does not contain. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Timofei Larkin
|
3852ba8774 |
ci(api-gate): report name declarations the API server would refuse
The gate read a name declaration only when it was an object, and diffed only the constraint keywords it knew. A declaration turned into a string or null, or a misspelled keyword, therefore looked like no change at all. Those are exactly the inputs on which the API server refuses every create of the kind, the harshest outcome a schema change can have. The gate now runs a changed declaration through the same parser the server uses and reports it when it cannot be enforced. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Timofei Larkin
|
180030be72 |
fix(api): refuse only what a name declaration actually breaks
Three cases refused creates of a kind, or were documented as refused, without the declaration being at fault. A schema that is not a JSON object was reported as an invalid x-cozystack-name, although such a schema has no declaration to blame, and the spec schema's own loader already reports it. It now declares nothing, so creates keep working and a kind with a legacy cap keeps it. Any unknown key inside a declaration took the kind offline, so a generator adding a title or an example would do the same. Keywords that constrain nothing, the JSON Schema annotations and x- extensions, are now ignored. A constraint keyword the server does not implement is still refused, because accepting it would drop it silently. The docs said a declaration no name satisfies is refused. That holds for lengths only: a pattern is checked to compile, not to match anything. The docs now say so. The parser moves to pkg/apis/apps/validation so the API review gate can reuse it rather than keep a copy of its rules. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> |
||
|
Timofei Larkin
|
146e06e75e |
feat(api): take Application name limits from the application's own schema (#4367)
## What this PR does
The aggregated API server capped two kinds' names by knowing what their
charts render. A `Kubernetes` cluster name was held to 32 characters
because every worker pool is a `KubernetesNodes` release named
`kubernetes-nodes-<name>-<pool>`, and the default `md0` pool has to fit
the 53-character Helm release name limit. A `Kafka` name was held to 44
because the KRaft controller pod `kafka-<name>-c-<hash>-<id>` has to fit
the 63-character DNS-1123 label limit. The arithmetic is right, but it
is the charts' knowledge living in the platform: the next application
with a sub-resource budget needs another `if r.kindName == ...` branch,
and an application served from outside this repository — the whole point
of `ApplicationDefinition` — cannot get one at all.
An application now declares the limits on its own name in its own
schema, and the server applies whatever it finds there for whatever kind
the definition describes. The chart's `values.yaml` carries a `## @name`
annotation, `cozyvalues-gen` v1.7.0 publishes it at the root of
`values.schema.json` as `x-cozystack-name`, and `hack/update-crd.sh`
carries it into the `ApplicationDefinition`:
```yaml
## @name {string} - Cluster name. Every worker pool is a separate KubernetesNodes release named `kubernetes-nodes-<name>-<pool>`, so the name must leave room for the default `md0` pool inside the 53-character Helm release name limit.
## @maxLength 32
```
```json
"x-cozystack-name": {
"type": "string",
"description": "Cluster name. Every worker pool is a separate KubernetesNodes release named `kubernetes-nodes-<name>-<pool>`, so the name must leave room for the default `md0` pool inside the 53-character Helm release name limit.",
"maxLength": 32
}
```
`maxLength`, `minLength` and `pattern` are honoured; `pattern` is
unanchored, as JSON Schema specifies. A declaration only tightens:
`prefix + name` must still fit the 53-character Helm limit. A rejected
name is answered with the chart's own description, or with "limit
declared by the <Kind> application" when the chart gives none.
The schema is untrusted input — definitions from other repositories are
written by hand — so a declaration that cannot be enforced as written
(not an object, a type error, an unknown keyword such as a misspelled
`maxLenght`, `maxLength` below 1, a negative `minLength`, `minLength`
above `maxLength` or above the Helm budget, a pattern that is not RE2)
refuses every create of that kind with `configuration error: the <Kind>
ApplicationDefinition declares an invalid x-cozystack-name: …`, and logs
the same at start-up. Dropping the declaration would admit names the
chart cannot render; applying it would reject every name and blame the
user. Other kinds are unaffected, and existing objects of the affected
kind can still be read, updated and deleted.
An upgrade brings `cozystack-api` up before `kubernetes-rd` and
`kafka-rd` re-render (`kubernetes-rd` depends on `cozystack-engine`), so
the new server can briefly read a definition that predates its
declaration. For exactly that case — a definition that declares nothing
— the two previous caps stay in `legacyNameCaps` as a floor, to be
removed once no supported upgrade starts from such a definition. That is
the only per-kind knowledge left in the name checks.
Also in this PR:
- The `pre-commit` job pins `cozyvalues-gen` v1.7.0, the first release
that understands `@name`
([cozystack/cozyvalues-gen#33](https://github.com/cozystack/cozyvalues-gen/pull/33)).
- The API review gate now diffs the root `x-cozystack-name` of every
definition as `metadata.name`, so a chart that lowers its cap or adds a
pattern is reported like any other tightened field. On this PR it
reports the Kubernetes and Kafka caps as added; their effective limits
are unchanged.
- The migration 56 comment that pointed at the removed
`maxKubernetesClusterName` now points at the chart's declaration.
Not in scope, because none of it is a static limit a chart can declare:
the `Tenant` name pattern in `pkg/apis/apps/validation`, the computed
tenant-namespace length check (it depends on the parent namespace at
request time), the tenant quota hierarchy, and the Phase 2 removed-field
warning for `Kubernetes`.
### How it is tested
`TestNameCapComesFromTheShippedSchema` reads the shipped `kubernetes`
and `kafka` ApplicationDefinitions off disk, builds storage through
`NewREST` as the server does, and checks the cap and the chart's
explanation; it then rewrites `maxLength` inside the shipped schema and
checks the limit moves with it, which a cap held in code would not.
`TestInvalidNameSchemaFailsClosed` covers each malformed declaration
through the same path, `TestLegacyNameCapsCoverUndeclaredDefinitions`
pins the upgrade floor and that a declaration replaces it, and
`TestClassifyNameDeclaration` covers the gate.
### Downstream repositories
Two repositories are affected and neither follow-up is filed yet, so no
box is ticked.
- `cozystack/website` — `content/en/docs/next/development.md` documents
the `values.yaml` annotations and needs `@name`;
`content/en/docs/next/cozystack-api/application-definitions.md`
describes `openAPISchema` and needs `x-cozystack-name`.
- `cozystack/external-apps-example` — it pins no `cozyvalues-gen`
version and regenerates against whatever is current; nothing in it
declares a name constraint, so it is unaffected until it chooses to.
- [ ] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:
- [ ] [cozystack/community](https://github.com/cozystack/community) -
follow-up:
### Release note
```release-note
feat(api): Application name limits now come from the application's own schema instead of being hardcoded per kind in the aggregated API server. A chart declares them with a `## @name` annotation in `values.yaml`, which cozyvalues-gen v1.7.0 publishes as `x-cozystack-name`; the API server enforces `maxLength`, `minLength` and `pattern` on `metadata.name` for any kind, answers a rejected name with the chart's own explanation, and refuses creates of a kind whose declaration it cannot enforce with a configuration error. The Kubernetes (32) and Kafka (44) name caps are unchanged in effect and are now declared by their charts.
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* Applications can declare minimum and maximum name lengths and
formatting patterns.
* Kubernetes cluster names are limited to 32 characters, and Kafka
cluster names to 44 characters, to meet generated resource naming
limits.
* Names that violate declared constraints are rejected with an
explanatory message. Tighter name constraints are also reported as
breaking changes.
* Declared maximum lengths cannot exceed the platform’s existing Helm
release-name budget.
* **Documentation**
* Added guidance on declaring and enforcing chart-specific resource-name
limits.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
Andrey Kolkov
|
d50385f310 |
fix(backups): tailor the Velero rename refusal to VMDisk and in-flight jobs
The refusal pointed every kind at "the application's engine-native strategy", but VMDisk, the only non-VMInstance kind the default BackupClass routes to Velero, has none. Offer that remedy only to kinds that can have one. The check also runs on every reconcile, so a rename RestoreJob that was already Running when the controller picked up the check is failed after its Velero Restore has completed. That job now says the application may already sit in the target namespace under its source name (a retry that drops the rename would otherwise trip failIfTargetExists on that copy without explanation), and its resource-modifier ConfigMap is released right away instead of at the job's deletion. Assisted-By: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
11bac54f93 |
test(backups): pin both sides of the Velero rename guard
The guard's regression cases only exercised renames, so dropping the IsRenamed conjunct (failing every non-VMInstance restore) or keying the check on targetApplicationRef.kind instead of the backup's kind left the package green. Add the inputs the guard must let through, a VMDisk copy that keeps its name and a VMDisk in-place restore without targetApplicationRef, and a VMDisk backup restored with a VMInstance target kind, which must still be rejected by the source kind. Assisted-By: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
Andrey Kolkov
|
ff52aeb44e |
chore(etcd-operator): bump to v0.5.6 (#4425)
## What this PR does Bumps the vendored `etcd-operator` from **v0.5.5** to **[v0.5.6](https://github.com/cozystack/etcd-operator/releases/tag/v0.5.6)**. v0.5.6 is a controller-only bug-fix release; the two changes are: - **Terminal-phase `EtcdMember` Pods are replaced** ([etcd-operator#370](https://github.com/cozystack/etcd-operator/pull/370)). A graceful node shutdown SIGTERMs etcd, which exits 0, and the kubelet parks the Pod in `Succeeded` without deleting it. The operator manages bare Pods and only recreated on `NotFound`, so after a whole-cluster reboot every member stayed down until the Pods were deleted by hand. The reconciler now deletes an owned Pod in a terminal phase and lets the next pass recreate it against the same PVC (or, for memory-backed members, through the existing pod-loss re-add path). - **`EtcdCluster` status stays truthful while etcd is unreachable** ([etcd-operator#369](https://github.com/cozystack/etcd-operator/pull/369)). On a converged cluster the learner-promote and auth reconcile paths returned their transient requeue before `updateStatus`, so `Available`/`Degraded` and `readyMembers` froze at the last healthy value and a fully down cluster kept advertising `Available=True`. The status write now always happens, with the requeue carried through it. Package changes are pin-only: - **`etcd-operator`**: `appVersion` → v0.5.6 (image tag derives from it), `ETCD_OPERATOR_REF` → v0.5.6, `tests/deployment_test.yaml` image pins → v0.5.6. - **`etcd-operator-crds`**: `ETCD_OPERATOR_REF` → v0.5.6. `make update` at the new ref produces an empty diff: the five CRDs are byte-identical to v0.5.5, and `charts/etcd-operator/files/manager-role-rules.yaml` did not change, so `templates/rbac.yaml` is untouched. Verified locally: `helm unittest` 18/18 (`etcd-operator`) and 1/1 (`etcd-operator-crds`), `helm lint` clean on both, `helm template` renders `ghcr.io/cozystack/etcd-operator:v0.5.6`, and that tag resolves in ghcr.io. ### Screenshots n/a — no UI changes. ### Downstream repositories Walked the trigger map against the diff. The change is confined to `packages/system/etcd-operator{,-crds}` and touches only version pins: etcd-operator is a system component (not a `packages/apps`/`packages/extra` package the website and TF provider track), no CRD, schema, values, `hack/`, installer or platform values changed. - [x] No downstream repository is affected by this change ### Release note ```release-note chore(etcd-operator): bump to v0.5.6, which replaces EtcdMember Pods left in a terminal phase after a node shutdown and keeps EtcdCluster Available/Degraded conditions truthful while etcd is unreachable ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Updates** * Updated the etcd operator to version 0.5.6. This version is used consistently across the operator deployment and its associated custom resource definitions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
IvanHunters
|
380c446cda |
fix(monitoring): pin the scrape size rendering and sharpen the alert
The per-job limit replaces the global flag downward as well, so an install that raised promscrape.maxScrapeSize past it through extraArgs has to raise this value too; say so where the value is set. Pin that a numeric value still renders as a string, which the CRD field requires, and pin the alert severity. Name the cluster in the alert and say where the apiserver value lives, since a guest cluster cannot set it. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Andrey Kolkov
|
88073bb209 |
fix(backups): fail Velero RestoreJob rename for non-VMInstance kinds
A RestoreJob that set targetApplicationRef.name to a new name and options.targetNamespace to another namespace reported Succeeded for any Velero-backed application, but only a VMInstance was actually renamed. postRestoreRename looks up the HelmRelease under the vm-instance- prefix, gets NotFound for every other kind and returns nil, so a Postgres, MariaDB, ClickHouse, MongoDB or standalone VMDisk copy came up under the source name with no error surfaced. Renaming those kinds through the Velero strategy cannot work: DataUpload restores volumes under their original names, and only a VMInstance keeps using them after its HelmRelease is renamed, because its disks keep their names and CDI adopts the restored PVCs. A renamed database Cluster or VMDisk renders fresh volumes and comes up empty. Reject the rename up front for every kind other than VMInstance, before the RestoreJob enters Running, with a message that points at omitting targetApplicationRef.name or using the engine-native strategy. The existing same-namespace guard keeps precedence, so its message is unchanged. Closes #3537 Assisted-By: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
IvanHunters
|
a17c5b2667 |
feat(monitoring): alert when vmagent refuses an oversized scrape
A target whose response exceeds the scrape size limit reports up == 0, which is indistinguishable from the target being down: TargetDown and KubeAPIDown fire and nothing names the real cause. Alert on vm_promscrape_max_scrape_size_exceeded_errors_total so the first alert says what is wrong. The counter has no target label, so the description points at the vmagent targets page to find the target. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
4a268f6c01 |
fix(monitoring): give the kube-apiserver scrape its own size limit
kube-apiserver's /metrics grows with the number of resources and CRDs, and on a full install it outgrows the global 32MB -promscrape.maxScrapeSize. vmagent then refuses every scrape of it, so up is 0 and KubeAPIDown and TargetDown fire against a healthy API server while every rule over API server metrics goes blind. Set max_scrape_size on the apiserver endpoint instead of raising the global flag. vmagent uses it in place of the flag for that job only, so the 32MB guard for every other target stays as it is. vmagent reads the whole uncompressed body into memory even with streamParse, so the per-job value is a memory ceiling and stays finite (128MB), exposed as vmagent.apiserverMaxScrapeSize. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Andrey Kolkov
|
e60fbc3f37 |
chore(etcd-operator): bump to v0.5.6
Controller-only release: EtcdMember Pods that reach a terminal phase are replaced instead of being left behind, and EtcdCluster status is no longer rewritten while etcd itself is unreachable. No CRD, RBAC or values changes; re-vendoring the CRDs produces an empty diff. Assisted-By: LLM Signed-off-by: Andrey Kolkov <androndo@gmail.com> |
||
|
IvanHunters
|
70ea242e1b |
docs(vm-instance-rd): trim the disable-wait comment and correct the mechanism
The annotation comment repeated the PR description and the test header; two lines on why are enough. The hang comes from the kstatus generation check in cli-utils, not the CEL status evaluator, since vm-instance sets no healthCheckExprs. The config-hash rollout runs in cozystack-controller, not cozystack-operator. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
22f2d76ead |
test(controller): pin the invalid-annotation and kind-selector paths of the wait carry-over
The reconciler skips a disable-wait annotation it cannot parse, and it selects releases by application kind. Removing either left the suite green; each now has a test that fails without it. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
5f379577d5 |
fix(controller): carry the readiness-wait setting to existing releases
A HelmRelease created before its ApplicationDefinition carried release.cozystack.io/helm-install-disable-wait keeps waiting on readiness, because cozystack-api only reads that annotation while building a release and an existing release is not rebuilt until its Application is written through the apps API again. Where the wait is what broke, that is every existing instance of the kind at once, each looping upgrade and timeout, and rewriting them by hand is the operator's only way out. The definition already drives chartRef, valuesFrom and labels on releases that exist, so drive the wait from there too, in both directions: the API rebuilds the whole spec on every write and emits DisableWait=false once the annotation is gone, so a release left disabled would drift back anyway. An absent install or upgrade block behaves as DisableWait=false already and is only materialised when the definition asks to disable the wait, which keeps a release nothing asks to change from being rewritten. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
IvanHunters
|
1ed32aec58 |
fix(controller): roll cozystack-api when a release annotation changes
The ApplicationDefinition config hash was built from name and spec alone, so a definition whose whole change is a release.cozystack.io/* annotation left the hash untouched and cozystack-api kept running with the configuration it read at its last start-up. Those annotations are only ever read there, once, in buildResourceFromCRD, which makes the config-hash rollout the single mechanism that delivers a change in one: without it the annotation is inert until the api pod restarts for an unrelated reason, and an inert annotation is indistinguishable from one that was never delivered. Hash the release.cozystack.io/* annotations alongside the spec. Only that prefix, because kubectl and Helm rewrite annotations of their own on every apply and hashing those would roll the api Deployment on every reconcile. Assisted-by: LLM Signed-off-by: IvanHunters <xorokhotnikov@gmail.com> |
||
|
Aleksei Sviridkin
|
45f5698bce |
fix(cdi): validate kubevirt-cdi values against a schema (#4417)
## What this PR does This gives the kubevirt-cdi chart a values schema, so a bad clone strategy fails when the chart renders instead of when the CDI object is applied. Today a typo like `csi_clone` renders fine, the CDI CRD refuses it on apply, and the operator finds out from a failed HelmRelease. The schema is generated by `cozyvalues-gen` from annotations in `values.yaml`, through a new `make generate` target, the same way `talos-log-collector` does it. There is no README here, so the target only writes the schema. The clone strategy is checked with a pattern, not with the CRD's enum. The chart uses the empty string to omit the field and let CDI pick a strategy per StorageProfile, and the CRD enum has no member for that. `cozyvalues-gen` also can't emit an empty enum value. So the accepted values are `copy`, `snapshot`, `csi-clone` and `""`. A null still works as before. The key has a chart default, so Helm drops a null for it before validation, and the template omits the field. `uploadProxyURL` only has to be a string. The new test suite refuses `csi_clone` and values of the wrong type, and accepts each of the allowed values and null. Closes #4141 ### Screenshots No UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(cdi): kubevirt-cdi now validates its values against a schema, so an invalid `cloneStrategyOverride` such as `csi_clone` fails at render time instead of when the CDI object is applied. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Configuration** * Chart values are now validated to ensure the upload proxy URL is a string, importer resources are provided as an object, and clone strategy uses a supported value: `copy`, `snapshot`, `csi-clone`, or an empty value. * Existing defaults for these settings remain unchanged. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
5e460605c2 |
fix(cdi): drop the ExpandDisks gate from the CDI object (#4415)
## What this PR does This drops a feature gate from the CDI object that CDI never reads. `ExpandDisks` sits in `spec.config.featureGates` in `packages/system/kubevirt-cdi/templates/cdi-cr.yaml`, but it is a KubeVirt gate. CDI v1.66.1 has no gate with that name. It looks a gate up by exact name, so an extra entry is read and ignored. The CRD types the list as plain strings, so nothing rejects it either. Disk expansion comes from KubeVirt, where `ExpandDisks` is GA at v1.9.0, the release the platform ships. I left the KubeVirt CR as is; #4029 tracks the GA gates in that list. The gate list now has its own test case, pinned whole to what upstream's `cdi-cr.yaml` ships. Adding `ExpandDisks` back turns it red. `make update` only checks the upstream file against its pinned hash and writes nothing under `templates/`, so it can't bring the line back. Closes #4112 ### Screenshots No UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(cdi): stop listing the KubeVirt `ExpandDisks` feature gate on the CDI object, where CDI ignored it. Disk expansion is unaffected. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Configuration** * CDI now enables only the `HonorWaitForFirstConsumer` feature gate. The `ExpandDisks` gate is no longer included in the configuration. * **Tests** * Updated configuration checks to verify the exact feature-gate list and ensure it matches the gate CDI supports. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
25d4a8c951 |
fix(kubevirt)!: evict non-migratable VMs instead of blocking the drain
The cluster-wide evictionStrategy was LiveMigrate. For a VMI that is not live-migratable (a ReadWriteOnce disk, a passthrough GPU, a host device), virt-api's eviction webhook answers every eviction with 429, so a node drain retries forever and never completes. LiveMigrateIfPossible keeps live migration for every VMI that can migrate and lets the eviction through for the rest. The VM is shut down and started again according to its runStrategy. A VM whose disk is bound to the drained node stays down until the node returns. KubeVirt writes the cluster strategy into a VMI's spec when the VMI is created, so a VMI started before the upgrade keeps LiveMigrate and still blocks the drain until it is restarted once. A drain and a launcher update after a KubeVirt upgrade go through the same eviction, so the workload updater's Evict fallback shuts down non-migratable VMs started under the new strategy on every later KubeVirt upgrade, where it used to get the same 429. Evict stays anyway: for kubernetes-nodes workers, which use evictionStrategy External, that eviction marks the VMI for evacuation, CAPK drains the tenant node and deletes the VMI, and the VM starts it again on the new launcher. The test pins the method list so the fallback is not dropped to avoid the shutdowns. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
1f30ac4598 |
fix(tests): let the mariadb waits cover a first boot (#3769)
## What this PR does Closes #3741 Several waits for a database HelmRelease to become Ready gave up before the release could: the mariadb, mongodb, rabbitmq and clickhouse backup walkthroughs waited 300 seconds, and both mariadb Chainsaw suites gave their HelmRelease-ready assert 6 minutes. A generated release carries a 10-minute install timeout (`pkg/cmd/server/start.go:91`), so a failed install was reported as a bare timeout, because the release had not yet recorded why it failed, and for mariadb a first boot the startup probe was still willing to wait for failed the harness instead. A wait on such a release has two floors and has to beat the larger. The first is the app's own first boot: cozystack-api leaves `spec.waitStrategy` unset and helm-controller defaults it to `poller`, so the install waits through kstatus, and kstatus maps the operator CR's `Ready=False` onto `InProgress`. For mariadb that boot may take the chart's 310-second startup budget (`startupProbe.failureThreshold: 30` in `packages/apps/mariadb/templates/mariadb.yaml` with the operator's probe defaults of 20 and 10); mongodb, rabbitmq and clickhouse set no startup probe. The second is the install timeout. At or below it the waiter is the one that expires, because its clock starts when the step runs and the install's only once the Application has been reconciled and the chart artifact fetched. The install timeout binds in every case here, so the walkthrough waits go to 660 seconds and the mariadb asserts to 11 minutes. Raising a wait inside a walkthrough spends the Chainsaw op that runs the script, and an op that runs out SIGKILLs the process group and loses the script's error. Each op therefore grows by exactly what the waits it reaches gained: mariadb's round-trip from 30 to 42 minutes, rabbitmq's from 30 to 42, mongodb's from 30 to 36 (its round-trip runs with `SKIP_RESTORE`, so only the source wait counts), and clickhouse's from 60 to 72, which still sits above the sum of every inner ceiling. That bounds a failure at a single wait, not a run that is slow at every stage and then fails late; bounding that needs a phase budget, not a larger literal. postgres keeps its 360-second waits and every walkthrough keeps its 300-second bucket wait: whether a suite pays the extra minutes on every red run is a per-suite call. The mariadb endpoint asserts keep their job. A freshly applied CR carries no conditions and kstatus reads it as current, so the install wait can end before any pod is up; an endpoint address closes that window. That is also why the `wait_for_field` on the CR stays in the walkthrough. `wait_hr_ready` now dumps every condition and the release history on timeout and on `Stalled=True`, instead of the single Ready message a scheduled retry can overwrite before anyone reads it. It also stops printing one `(none recorded)` line for different answers: a release that never appeared, a release deleted during the wait, a lookup kubectl could not answer, a failed history read and an empty history each say so, with kubectl's own stderr kept, and a release seen earlier still gets its dump when only the last lookup failed. The change goes to every byte-identical copy of the helper, in `examples/backups/{clickhouse,mariadb,mongodb,postgres,rabbitmq}/00-helpers.sh`, and they stay identical. `hack/mariadb-first-boot-waits.bats` pins the mariadb budgets against the tree. It derives both floors from their sources instead of pinning literals, so raising `failureThreshold` past the install timeout, or changing the install-timeout default, reds it; each source has a test that refuses to proceed when it cannot be read, including the ApplicationDefinition, whose two override annotations would each invalidate the derivation. It holds the round-trip op at a fixed remainder over the ceilings up to and including the source wait, bucket stage and S3 preflight included, and at a fixed slack once both mariadb waits are paid, so raising a wait without the op reds. The extractors are also tested against fixtures, because the tree cannot tell a correct extractor from a broken one. Beyond mariadb, the file runs every exit branch of `wait_hr_ready` in each copy against a stubbed kubectl, and pins the copies byte-identical. The population is every definition in the tree found by grep, minus two named exceptions held to still differ, so a new copy joins without editing the test and an exception whose copy converges goes red: - `examples/backups/redis/00-helpers.sh` is written in the compact one-line style of the rest of that file, so converging it rewrites its shape rather than applying the shared body. - `examples/backups/kafka-metadata/00-helpers.sh` is a helper of its own with the same diagnostic gap on timeout (it prints no reason at all), and closing that is a change to kafka-metadata. `docs/agents/e2e-testing.md` replaces the reviewer checklist's 5-6m standard and its list of exceptions, which the tree had outgrown, with the floor a wait has to beat and the rule that a longer wait carries its reason in-line. The derivation gets a section of its own, opening with the rule in one line. For a release an app chart renders itself the floor is `spec.install.timeout`, then `spec.timeout`, then helm-controller's 5m default, and the nested `<name>-system` releases never take cozystack-api's 10m. The harbor comments name the floor each of their asserts sits against, and the opensearch comment stops citing the removed standard. Adding `hack/mariadb-first-boot-waits.bats` does not escalate the E2E selection: `hack/[^/]+\.bats$` sits in `inert_config_pattern` in `hack/select-e2e.sh`, and `full_suite_pattern` escalates only `hack/e2e-*.bats`. On this changeset `hack/select-e2e.sh` selects `clickhouse harbor mariadb mongodb opensearch postgres rabbitmq`. What this costs is paid only on a failing run; the raised numbers are ceilings. Summing the timeouts stated on each suite's own steps, mariadb goes from 92m to 114m, clickhouse from 116m to 128m, rabbitmq from 67m to 79m and mongodb from 95m to 101m, before the untimed steps that fall back to the defaults in `.chainsaw.yaml`. clickhouse is the most expensive suite by that sum. All of them compete for the 215m `timeout-minutes` of the whole `e2e` job, which also covers image download, sandbox preparation and the Cozystack install, and a job cancelled at the cap loses both its log and `cozyreport.tgz`. This does not fix the failure that led here. In the run that prompted the investigation the mariadb replica never opened port 3306 at all, spending its whole 310-second budget inside `mariadb-install-db` before being killed, and no wait budget would have saved it. That defect is #3744. What this fixes is a second, independent defect found while reading the same code. One thing here is argued rather than measured: the mechanism is checked against helm-controller and kstatus, and every number is derived from the tree, but no run ID pins that it is what any particular red run hit. Both floors justify 660s whatever the cause was, so being wrong about the cause leaves slack rather than breakage. ### Not in this PR - This PR retires the mariadb, mongodb, rabbitmq and clickhouse walkthrough waits and the two mariadb asserts. The rest is tracked in #4404: the other Chainsaw HR-Ready asserts under their floor, the harbor asserts, the 300-second bucket waits, the postgres waits, and the redis and kafka-metadata diagnostic gap. - The clickhouse round-trip comment says every wait fails fast, `wait_hr_ready` on `Stalled=True`, "so a genuinely broken backup reports in seconds". A release whose install is still running is neither Ready nor Stalled, so that holds for a stalled release and not for an install that has not finished. The sentence predates this change. - `hack/mariadb-first-boot-waits.bats` is large for what it guards (two floors, two ratchets on the op, and the helper identity pin), with many fixture tests on its own extractors. Trimming it to that core is a separate decision. - Two comments in that file state facts about the mariadb suite that are true today and that nothing checks: three script ops carry a minute timeout, and the only one ahead of the round-trip is the 8m pre-clean. - The mariadb op comment says a bound on the whole flow "needs a phase budget (docs/agents/e2e-testing.md)"; the doc describes it as bounding the collection phase as a phase (`COZY_DIAG_PHASE_BUDGET`), so the pointer would resolve faster if it named that bullet. - Cosmetic: the behavioural test's loop body is not indented under its `for`, one line of the bats header runs past the width of the rest, and the checklist's sentence under the example assert ("what this block demonstrates least") reads awkwardly. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map in `docs/agents/contributing.md` against the diff, file by file, for all eighteen files. The five `examples/backups/*/00-helpers.sh` files change the diagnostics of `wait_hr_ready`, and the mariadb, mongodb, rabbitmq and clickhouse walkthrough scripts change wait budgets; the manifests the walkthroughs apply and the flows they document are unchanged, so no website page restates anything that moved. Four Chainsaw suites (mariadb, clickhouse, mongodb, rabbitmq) change timeouts and comments, and the harbor and opensearch suites change comments only. `docs/agents/e2e-testing.md` changes the reviewer checklist entry and adds the sizing section. `hack/mariadb-first-boot-waits.bats` is a new file picked up by the existing `BATS_UNIT_FILES` wildcard. `hack/package.mk`, `hack/common-envs.mk` and `hack/update-crd.sh` are untouched, and no `values.schema.json`, `values.yaml` default, version enum, `ApplicationDefinition`, CRD, namespace or release asset is touched. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(tests): the MariaDB, MongoDB, RabbitMQ and ClickHouse backup examples and the MariaDB Chainsaw suites now wait for a HelmRelease past the release's own install timeout, so a failed install reports its own reason instead of a bare timeout, and the examples' HelmRelease wait now tells a release that never appeared, one deleted mid-wait and a kubectl that could not answer apart. ``` |
||
|
Aleksei Sviridkin
|
9c97a7a7ac |
fix(cdi): validate kubevirt-cdi values against a schema
Without a schema, a cloneStrategyOverride outside the CDI enum, such as csi_clone, renders a CDI object the API server refuses on apply, so the mistake shows up as a failed HelmRelease rather than at the value. The field is constrained by a pattern, not by the CRD's enum. The empty string is the chart's way to omit the field and let CDI pick a strategy per StorageProfile, the CRD enum has no member for it, and cozyvalues-gen cannot emit an empty enum value. The key has a chart default, so Helm drops a null for it before validating, and a null keeps rendering as an omitted field. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Andrey Kolkov
|
7ce85a2f7f |
test(kafka): pin the strategy script's guards with a hermetic bats suite
The in-Pod script had grown eight fail-closed guards across as many rounds, and the only thing exercising any of them was a 40-minute cluster test that drives the happy path plus one refusal. The partition listing guard in particular - the whole of the previous round's second fix - could be deleted with nothing in the tree going red. Add hack/kafka-topic-backup-strategy.bats on the shape of hack/kafka-backup-script.bats: render the strategy through a stub kubectl exactly as step 01 applies it, lift the container body out of it, and execute it against stubbed kafka-*.sh, a stubbed curl and a directory standing in for S3, with fault injection for the broker behaviours the guards exist for. Deleting that listing guard now fails one case; the script gains BIN/SCRATCH/CA_FILE overrides so it can run outside the Pod, each defaulting to the Pod's value. The same rounds' smaller findings: the manifest's topic name is held to Kafka's charset like the topics parameter, since it reaches unquoted expansions with globbing on; the shape check after --create compares the replication factor as well as the partition count, and refuses a replication factor below the topic's effective min.insync.replicas before replaying, because kafka-console-producer only logs a rejected acks=all write and the run would otherwise end at the count check with the cause unnamed; the manifest's replication factor must agree across a topic's lines; topic_records carries its own || return 1 on each command, as a function called as the operand of || runs with errexit off; the CA lookup in step 03 distinguishes a denied read from an absent Secret, and the README's persona column says the copy needs the admin; the e2e comment no longer claims the 2-label endpoint fails the serving cert's SANs, which include '*.tenant-root'. Rebased onto main, which moved the chart to KRaft: the demo's Kafka CRs drop the zookeeper block the chart no longer has. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-By: LLM |
||
|
Aleksei Sviridkin
|
0c358addfb |
fix(cdi): drop the ExpandDisks gate from the CDI object
ExpandDisks is a KubeVirt feature gate. CDI declares no gate by that name and matches gate names exactly, so the entry was accepted and ignored. Disk expansion is a KubeVirt feature, GA there at the release the platform ships, so listing the gate on the CDI object points a reader tracing where expansion comes from at the wrong component. The feature-gate list is pinned whole in its own case, so a gate added beside the one upstream ships turns the suite red. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la> |
||
|
Aleksei Sviridkin
|
2b5770cb4a |
fix(vm-instance)!: refuse resources that cannot size the VM (#4405)
## What this PR does A vm-instance could ask for half a core and get one, with no error anywhere. `resources.cpu` and `resources.sockets` are typed as quantities, but they end up in `domain.cpu.cores` and `domain.cpu.sockets`, which KubeVirt declares as unsigned integers. The chart cast them with `int64`. That turns `500m` or `1.5` into `0` without an error, and KubeVirt boots a zero core count as one core. The chart now reads these two fields the way an integer field reads them. It decodes the raw value as YAML and stops the render unless the result is a positive whole number that fits in uint32. So `2`, `"2.0"`, `"+2"` and `"1e1"` render as the integer they decode to. `500m`, `2000m`, `1k`, `"1.5"`, `0`, `"0"`, `"-2"`, `"010"` and `"4294967296"` stop the render with an error that names the field. A leading zero is refused because YAML reads `010` as octal 8. The check looks at presence, not truthiness, so an unquoted `0` is refused instead of being read as unset. It is the same rule #4386 uses for kubernetes-nodes. Memory gets a presence check too, which covers the case from the issue thread. A quoted `"0"` was truthy and rendered `requests.memory: "0"`, while an unquoted `0` was falsy and got dropped. Both spellings of zero, and any negative amount, now stop the render. Any other value passes through unchanged, because KubeVirt takes memory as a quantity. That includes an amount below one byte, such as `500m` or `1e-3`, which renders as it does on `main` today; this change does not refuse it. The check lives in `virtual-machine.domainResources`. The VM spec, the instance type decision and the update hook's patch all take their values from that helper. The tests that pinned `cores: 0` as the old behaviour now expect the refusal, and the helper comments about the truncation are trimmed. This is a breaking change. A release that carries one of these values fails its next upgrade until the value is fixed: a cpu or sockets that is not a positive whole number within uint32 or has a leading zero, and a zero or negative memory. This includes a cpu or sockets set on its own next to an `instanceType`, which used to be ignored whatever it held. Some values change meaning instead of failing. A value like `"1e1"` or `"2."` was truncated to 0 by the `int64` cast, so the VM booted with one core. Now it gets the count the value spells, so that VM changes size on upgrade. I refuse these values instead of rounding them, because silent coercion is how `500m` became one core in the first place. The issue asks for a deprecation path, and I did not add a migration that rewrites stored values. It would have to guess what `500m` meant, and both options (one core, or dropping the field) hide the same mistake again. The HelmRelease upgrade retries without uninstalling, so the running VM stays up until someone fixes the value. #4386 makes the same trade-off. The check runs at render time, not at admission. The schema still accepts `500m`, so the API takes the object and the error shows up in the HelmRelease status. Narrowing the schema to integers, as the issue also proposes, would move the error to admission, but it needs a migration of stored values. I left it for a separate change. The field descriptions now say which values are accepted, and `make generate` carried them into the README, the schema, the Go types and the ApplicationDefinition. The new `tests/resources_values_test.yaml` covers the accepted and refused spellings for cpu, sockets and memory. `tests/instancetype_resources_test.yaml` gets a case for a lone cpu next to `cpuModel`. The whole vm-instance suite passes. Every refusal case in the new file, and the `"1e1"` case, fails against the current `main` template. Closes #4104 ### Screenshots No UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: I walked the trigger map against the diff. No field is added, removed or renamed, and no default or enum changes. Only three field descriptions change, and the website reference page is regenerated from the README at release time. ### Release note ```release-note fix(vm-instance)!: resources.cpu and resources.sockets must now be positive whole numbers, and resources.memory a positive quantity. A cpu or sockets that is not a plain whole number within uint32 (for example 500m, 1k or 1.5), or a zero or negative memory, now fails at render time with an error naming the field, instead of silently becoming a one-core VM or a zero memory request. A release carrying such a value fails its next upgrade until the value is fixed. A value such as 1e1 or 2., which used to be truncated to one core, now sizes the VM with the count it spells. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * VM sizing now accepts only positive whole-number CPU and socket values within the supported range. Fractional, millicore, zero, negative, and out-of-range values are rejected during rendering. * Memory values must be positive quantities. A CPU or socket value specified alone does not size the VM and is ignored; incomplete sizing is rejected where required. * **Documentation** * Updated VM instance resource guidance to clarify CPU and socket requirements, memory validation, and how socket values define vCPU topology. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Aleksei Sviridkin
|
99de79ff52 |
fix(cdi): pin the CDI release kubevirt-cdi is reconciled against and stop update overwriting it (#4107)
## What this PR does `packages/system/kubevirt-cdi` resolved its CDI version at run time from GitHub's `releases/latest` redirect and wrote the result straight over `templates/cdi-cr.yaml`. That file is not vendored. Upstream ships a bare CDI object; ours adds Helm templating for `cloneStrategyOverride`, `uploadProxyURL` and the worker-pod `podResourceRequirements`, the `ExpandDisks` feature gate, a `CriticalAddonsOnly` toleration, and the two RBAC documents that let a tenant clone from the golden-image namespace. I diffed the two at v1.66.1 to write that list, so it is complete as of that release rather than only hedged: upstream ships 283 bytes with one feature gate, two nodeSelectors and an `imagePullPolicy`, and we are a strict superset of it. Every entry arrived by hand, so `wget -O templates/cdi-cr.yaml` was not refreshing a vendored file, it was overwriting a first-party one. Same loss as the `rm -rf templates` that #4097 removes, arriving through the fetch instead of the wipe. `update` now pins the release the object is reconciled against, and fetches it only to confirm the bytes still hash to a digest I measured. Nothing is written under `templates/`. At the next CDI bump whose asset differs, that check fails, which is the point: upstream moving its own defaults is when the local copy needs a human to reconcile it, not an overwrite. The download is kept on a mismatch, since reconciling one starts by reading it. `RELEASE` is `v1.66.1`, which is the release `kubevirt-cdi-operator` vendors, so the pin and the deployed version agree. What the platform deploys is decided by that package, and it can move to another release without this file changing, so this line is pointed at it by hand. `update` then checks the two agree: it compares `RELEASE` with the `cdi-operator` image tag that package vendors and fails naming both versions when they differ, because otherwise its closing line would report a release the platform may no longer run. That is a check and not a derivation, so `RELEASE` stays a pin a human moves. Upstream publishes no checksum for the asset, so the pin comes from a download rather than from a published file: `a497f90de608c1df26f9ee4095289373f17132d74a2042ca03e00c17c964f8a7`. It catches an asset that moved under a tag that should be immutable. It is not authenticity verification, and the comment beside it says so. The digest command is chosen when `update` expands it, `sha256sum` when it is on PATH and `shasum -a 256` otherwise. Which one ran here is worth stating, because the usual claim about macOS is out of date: this machine picked `sha256sum`, and that binary is `/sbin/sha256sum`, a native Darwin one rather than GNU coreutils. It reports `sha256sum (Darwin) 1.0` and prints the same `<digest> <path>` format. The package already had a `test:` target, from `0418577f5`, so what it gains here is the `.PHONY` line that commit did not add. `hack/package.mk` declares only the nine targets it defines itself, so without it a file or directory named `test` or `update` beside the Makefile makes make consider that target up to date and skip the recipe in silence. The new suite pins the `ExpandDisks` gate, the toleration, `podResourceRequirements`, the `uploadProxyURL` override and the clone RBAC, and renders the upload-proxy Ingress and TLSRoute, each under the `gateway-enabled` value that selects it. The clone-strategy override is pinned by `tests/clone_strategy_test.yaml`, which is already on `main`. One consequence of the pin move is a claim rather than a number, so it gets its own line. Upstream carried `WebhookPvcRendering` from v1.63.0 through v1.65.0 and dropped it by v1.66.0; the local object never carried it, so the divergence ran both ways across those three releases and one way since. The comment says to re-derive the delta at each bump instead of trusting it, because that was a fact about a span of releases and not about CDI. `packages/system/kubevirt-cdi/Makefile` carried two identical `test:` targets, one from `0418577f5` and one appended by a later merge, so make printed `overriding commands for target 'test'` on every invocation in the package. The second sat inside the span this recipe rewrites, so it goes with the rewrite rather than being re-authored into it. The comment kept above it is the reason a reader should not add another. Closes #4157. ### Scope An earlier revision also pinned and tested `kubevirt-cdi-operator`. #3920 has since landed the two real bugs in that recipe, the patch strip level and `patches/tolerations.diff`, so that half is dropped here rather than merged twice. Pinning and testing the operator package is #4190. Named improvements left alone, so they read as choices. - The two upload-proxy tests each render their route under the `gateway-enabled` value that selects it and neither asserts the other is absent; the templates already settle that with `ne "true"` and `eq "true"` on the same variable, and `tests/standalone_render_test.yaml` already asserts each route absent under the values that select the other. So the package does cover it; these two cases just do not, and a third copy would not add anything. - The CDI object carries no version string for a rendered-output assertion, so `RELEASE` is checked by `make update` rather than by `make test`. - Neither route case pins the backend service port, 443 in both templates; that is outside what the suite claims. - The template-loss guard still counts `cdi-cr.yaml` as named, because the download line in `update` mentions the stem, so re-adding a wipe here would have it report two files instead of three; the guard's header already says a mention is not proof of a write, and the case needs a wipe this recipe no longer has. - The suite pins the local additions by design, and most of the upstream-derived half is pinned by nothing: `imagePullPolicy` and the two `nodeSelector` blocks have no assertion anywhere under `tests/`. `HonorWaitForFirstConsumer` is the exception and only as a side effect: `featureGates` is pinned whole, so that gate rides along with `ExpandDisks`. The rest are maintained by the same hand reconciliation the pin forces whenever the upstream asset changes, so a carry-over that drops one of them is green. Three asserts would close what is left. - One more, remote enough to name rather than code around: if the digest tool is missing entirely, `$$actual` comes out empty, the comparison against `CR_SHA256` fails, and the error text diagnoses a moved asset when the real cause is a machine with neither `sha256sum` nor `shasum`. Busybox ships the first and macOS ships the second, so nobody has hit it. Closing it means an executable line, which would then owe mutation coverage of its own, and that is disproportionate to a case with no known instance. The mechanism is written here so the next reader does not have to re-derive it from an error message that points the wrong way. - Separately, the per-package `.PHONY` gap this Makefile's own line argues against is not specific to this package: most package Makefiles do not declare their own `test:` or `update:` phony, and #4214 tracks that. - A key that does not exist yet is not held either: every item in the delta is pinned whole, but a NEW key appearing at `spec` or `spec.config` level passes unnoticed, and such a key can change behaviour rather than sit inert. I confirmed it on a copy extracted from this commit, appending `spec.config.filesystemOverhead.global: "0.99"` and getting a green suite. That is the shape to recognise at the next bump: not a changed value, which is caught, but a new sibling key beside the ones we pin. - And `spec.config.podResourceRequirements` is now pinned whole here and by four leaves in `tests/importer-resources_test.yaml`: the whole-map form is the stronger one and the leaves are a subset of it, so changing the default takes two edits and the second failure reads as a surprise until you know why both exist. - If the operator's image reference ever gains a digest (`:v1.66.1@sha256:...`), `${deployed##*:}` takes the digest's hex instead of the tag, and `update` fails with a loud false mismatch rather than passing silently. - Deriving `RELEASE` with `$(shell ...)` from the sibling manifest was suggested and declined on merits: a derived value is not a pin, the point is that a human chooses which CDI release this object is reconciled against, and computing it from whatever the neighbour last vendored hides that choice from the diff. The operator comparison lives in `update` and not in `test:`, even though `kubevirt-operator` ties its own manifest to the tree from `test:`. A mismatch calls for a hand reconciliation, and `update` is the recipe that asks for one; it is also where the success line that would otherwise name the wrong release is printed. I checked it on a copy extracted from this commit. With the operator's image tag moved to v1.67.0, `make update` exits 2 and names both versions. With the tags equal it passes. With the check's `exit 1` removed, the moved tag passes again, so the red comes from the check. Worth knowing when reading the mismatch path: `kubevirt-operator` deletes its download on a hash mismatch and this package keeps it. That is deliberate here, since reconciling a moved asset starts by reading it, but the two packages do read oppositely and neither file says so. ### Base and ordering This stacks on #4097 and should land after it. Both edit `packages/system/kubevirt-cdi/Makefile`, and this change builds directly on that one's edit to the same recipe, so it is based on the head of `fix/cdi-update-keeps-local-templates` rather than on `main`. GitHub computes mergeability against the PR's base branch, which here is that stack branch and not `main`. Both branches are rebased onto current `main`, which has not touched these paths since the previous merge-base. ### Screenshots No UI changes. ### Downstream repositories Walked the trigger map against the diff. Two entries come close and neither fires. The website triggers on developer tooling and package Makefiles, but `content/en/docs/next/development.md` documents `make update` only as "Update Helm chart and versions from the upstream source", which still holds, and its worked example is cilium, which this does not touch. Its `make test` line refers to the e2e sandbox in `packages/core/testing`, not per-package unit tests. `ccp` triggers on `hack/` layout and on what a make target does, and nothing under `hack/` is touched. Its `package-bump` skill already assumes a package's `update` recipe hardcodes the upstream version instead of resolving it at run time, and names cilium, kafka-operator, opensearch-operator and postgres-operator as examples. `kubevirt-cdi` was one of those counterexamples and now is not. Pinning `kubevirt-cdi-operator` the same way is #4190 and out of scope here. The skill handles vendored Helm charts, in-repo image builds and version enums, and a raw manifest download is none of those. Everything else in the map needs an app, a schema, an installer value, a variant or a release asset. - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(cdi): `make update` in kubevirt-cdi no longer overwrites the hand-maintained CDI object. It pins the CDI release that object is reconciled against instead of resolving GitHub's releases/latest at run time, and verifies the fetched asset against a pinned sha256 without writing under templates/. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Reliability** * Updates now use verified, pinned CDI releases to prevent unintended manifest changes. * Failed or checksum-mismatched downloads no longer overwrite existing configuration. * **Bug Fixes** * Preserved local CDI customizations during release updates. * Added validation for CDI resources, access controls, upload-proxy routing, and TLS configuration. * **Tests** * Added automated checks for incomplete template refreshes and generated Kubernetes resources. * Added chart tests covering CDI behavior, clone permissions, and upload-proxy networking. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Andrey Kolkov
|
5bd029c46a |
fix(kafka): restore with the captured replication factor and a complete listing
Record each topic's ReplicationFactor in the backup manifest, off the same --describe line the partition count already comes from, and recreate the topic with it on restore. The value used to be a BackupClass parameter defaulting to 1, and the Cozystack Kafka chart sets the broker-level min.insync.replicas to 2 as soon as kafka.replicas reaches 2: kafka-console-producer writes with acks=all, the broker rejects every write into a topic with fewer replicas than that, and the producer logs the rejection and exits 0. A restore onto any multi-broker cluster therefore landed zero records and failed the post-replay count with a message naming the wrong cause; in place, the documented delete had already taken the live topic with it. replicationFactor stays as an override for restoring into a smaller cluster, and a value the target cannot host now fails at --create in Kafka's own words. Hold the pre-replay emptiness check to the topic's partition count, as the backup side already does. kafka-get-offsets skips a partition it could not read and still exits 0, so a leader election in flight dropped the populated partitions from the listing, the surviving empty ones summed to zero, and the replay appended a full copy into a live topic - the exact outcome the check was moved ahead of the replay to prevent. Both restore-side counts now go through one helper that refuses a short listing. The data-file-versus-manifest check runs both ways before anything is created, and the replay walks the manifest topic by topic, announcing each one, so a retried Pod's log says which topics landed. The demo now runs two brokers with two-replica topics, which is the shape the chart puts min.insync.replicas=2 on; the restore steps assert the restored topic's replication factor, and step 06 submits a second RestoreJob against the populated topic and requires it to be refused with the topic untouched. Smaller findings from the same round: the topics parameter is held to Kafka's name charset (a literal "*" no longer path-expands) and refused on a repeated name, which produced a Succeeded backup no restore could balance; the strategy verifies S3 TLS against the seaweedfs CA projected into the <app>-backup-s3 Secret instead of curl -k, following the rabbitmq example, so the e2e endpoint moves to the .svc name the serving cert covers; the image lookup keeps digest-pinned references; the default namespace is tenant-root, the only one the demo ever ran in; step 05 caches the resolved Backup name for the restore steps; the decoy count subtracts begin offsets like the topic count; the bats test name carries its count again; and the scenario walk-throughs the sibling examples ship are added. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-By: LLM |
||
|
Andrey Kolkov
|
b141166cde |
fix(kafka): key begin-offset lookups on topic:partition
The begin-offset lookups matched on the partition index alone, so a line belonging to another topic could supply this partition's begin. The cut then shifts silently and the drain check cannot see it, because n is derived from that same wrong begin. Match on the whole topic:partition key in all four lookups, which holds whatever order the listing arrives in and no longer depends on the topic filter being pinned. Hold the tarball's file names to the manifest as well. The manifest is validated as untrusted input, but the replay derived its topic from the data file names, so a file naming a topic the manifest never listed was replayed into a topic that skipped the create, shape, emptiness and post-replay checks while the restore still reported success. Correct what the decoy is documented to prove. It reliably covers the offset listing, the in-place delete and the two record counts; it does not cover the two PartitionCount reads, because kafka-topics --describe yields topics in the admin client's result-map order rather than sorted, so which one their head -1 sees is not something a fixture can pin down. The previous text claimed every pin reddens the run, and explained the decoy's name by a sort order that does not govern that output. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-by: LLM |
||
|
Andrey Kolkov
|
17475f1ee5 |
fix(kafka): refuse a non-empty restore target before replaying
The replay appends and the driver retries a Pod that died mid-replay, so checking the record count only afterwards meant every attempt had already written another full copy into a live topic and the check merely reported the damage. Read the target's count before replaying and refuse a topic that already holds records, leaving it as the failed attempt left it; the post-replay check still verifies what landed. Name the decoy so it sorts before the demo topic. Both PartitionCount reads pipe --describe through head -1, so an unpinned describe reads whichever matching topic comes first: with the decoy sorting after the topic those two pins were never exercised and could be removed with the suite staying green. "-" precedes ".", so orders-v1 now sorts first and an unpinned describe reads its partition count instead. Also state in the README that an empty backup is refused on restore rather than merely uploaded, so the empty-tarball note does not read as if such an artifact were usable. Signed-off-by: Andrey Kolkov <andrey.kolkov@aenix.io> Assisted-by: LLM |