mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 22:13:18 +09:00
Aleksei Sviridkin
4b6b1dc2da
## What this PR does Closes #2909. The matcher keyed off `instanceType`, the domain sizing keyed off `resources`, and neither side looked at the other, so the chart could render both. KubeVirt refuses that, and the two halves of its rule are different shapes: `validateCPU` conflicts on `domain.cpu.cores` and `domain.cpu.sockets` only when they are non-zero, while `validateMemory` conflicts on `domain.resources.requests.memory` whenever the key is present. `values.yaml` ships `instanceType: "u1.medium"`, so clearing the field in the dashboard just falls back to that default. Fill in cpu, sockets and memory after clearing it and you get a VM the webhook rejects, naming fields the form gives you no way to clear. Resources that size the VM completely now suppress the matcher and size it directly. `packages/apps/kubernetes-nodes` already does this for its worker VMs. Beside an instance type, sizing that is only partial stops the render, naming what sized the VM and what did not; with no instance type in play there is nothing to conflict with, so that half renders as it does today. Letting resources win unconditionally would silently drop the instance type for a VM that sets memory alone, which is a different bug in the same place. Completeness is decided from what `domainResources` emitted, never from which keys the user set, and all three fields go through that one reading. The two readings come apart in more than one place. `resources.cpu` on its own emits nothing, so it conflicts with nothing and goes on rendering a plain instancetype-sized VM. cpu and sockets pass through `int64`, which yields 0 for a quantity the schema accepts, so `cpu: 500m` beside a whole sockets and a memory would read as complete from the values, drop the matcher, and hand KubeVirt a `cores: 0` the user never asked for. And a `resources` block nulled outright leaves `.Values.resources` nil rather than the chart's `{}`, so a raw field read is a render-time nil pointer where the emitted object is merely empty. The existence check and the pre-upgrade update hook read the same effective value. Keying the `lookup` on the raw one would fail a resource-sized render over an instance type it never references, which is the same confusing error this fixes. Reading `.Values.instanceType` in the hook would see the leftover value, skip the removal patch, and leave `spec.instancetype.revisionName` on a VM Helm had just given explicit domain resources. ### What changes in the rendered output | `instanceType` | `resources` | Before | After | | --- | --- | --- | --- | | set | unset | matcher only | unchanged | | unset | cpu, sockets and memory | domain cpu and resources | unchanged | | set | cpu, sockets and memory | matcher plus domain sizing, refused by KubeVirt | domain sizing, no matcher | | set | any subset with memory, or cpu with sockets | matcher plus a conflicting domain field, refused by KubeVirt | render stops, message names both sides | | set | cpu alone, or sockets alone | matcher only | unchanged | | set | cpu and sockets that both truncate to zero | matcher plus `cores: 0` | unchanged | | unset | anything short of all three | already refused by the existing check | unchanged | The rows that stay unchanged are deliberate. `virtual-machine.domainResources` writes `domain.cpu` only when cpu and sockets arrive together, so cpu alone emits no override at all. A `cores: 0` beside a matcher is not a conflict either, because `validateCPU` guards on non-zero, so KubeVirt admits it and the instance type sizes the VM. Both shapes render today and go on rendering. `cpuModel` is left alone for a related reason: it writes `domain.cpu.model`, which KubeVirt flags only when the instance type declares a model of its own, and no instance type shipped in `packages/system/kubevirt-instancetypes` declares one. ### Why no working release breaks Every shape that now fails at render was already being rejected by the KubeVirt validating webhook at admission, so it could not have produced a running VM in the first place. The failure moves from an opaque admission error to a render error that names the fields. Nothing that worked stops working. The unchanged rows above are measured. Each non-failing shape in the table, plus firmware on and off, an explicitly nulled `resources` block and the three update-hook branches, was rendered against a mocked cluster on both sides and compared document by document. Across 14 value shapes and 78 rendered documents, 77 come out byte-identical. The one that differs is the row the table marks as changed, which is also what the harness was checked against first, so it is known to be able to fail. ### Live-VM impact This changes a rendered VM spec, so an upgrade can resize or restart a running VM. Adding a complete `resources` block to a VM currently sized by an instance type drops its matcher, and the pre-upgrade hook patches `spec.instancetype` off the live object and writes the domain resources in its place. Cozystack runs KubeVirt with `vmRolloutStrategy: LiveUpdate` (`packages/system/kubevirt/templates/kubevirt-cr.yaml:125`), so KubeVirt applies what it can to the running VMI and marks the rest `RestartRequired`; I have not measured which half a given resize lands in, so plan the upgrade as if it restarts. Dropping the matcher drops the whole instance type, sizing and everything else it carries. Of the 69 shipped cluster instancetypes, 36 declare `dedicatedCPUPlacement`, 36 declare `hugepages` and 26 declare both. 20 declare NUMA passthrough, 8 declare `realtime`. A VM moved from `rt1.medium` to explicit resources loses those characteristics along with the type. Before this change that combination was rejected outright, so the VM never existed; now it is admitted, and the field help says so. ### Tests `tests/instancetype_resources_test.yaml` covers matcher only, resources only, both complete, each failing partial shape, the shapes that emit nothing, both cpu and sockets truncating to zero, `cpuModel` beside a matcher and beside complete resources, a stale instance type with and without complete resources, an explicit `instanceType: null`, an explicitly nulled `resources` block, and every branch of the update hook's instancetype selector, in both directions plus the two no-op cases. 24 cases in this package, 2127 across `make helm-unit-tests`, all green. Every case pinning new behaviour was checked against a reverted template to confirm it goes red; the rest pin behaviour this change deliberately leaves alone. A ten-mutation ladder over the helper, `vm.yaml` and the update hook leaves no survivor. The memory branch took the most work. Swapping its emitted read for a raw `.Values.resources.memory` looks like an equivalent mutant, since `domainResources` emits `resources.requests.memory` under the same truthiness condition. It is not: a `resources` block nulled outright leaves `.Values.resources` nil, and the raw read dereferences it. The case pinning that shape kills the mutant. The update-hook cases assert the `kubectl patch` command the Job will run, which is as far as a render-level test reaches. They do not show that KubeVirt accepts the resulting object, that helm-controller converges on a live VM whose `spec.instancetype` also carries a `revisionName` written by virt-controller, or which half of a resize lands as a live update rather than `RestartRequired`. That needs a cluster. The `dig` calls survive the shapes that reach them. `values.schema.json` rejects a non-object `resources` before render; Helm coalescing turns a nulled one into a nil `.Values.resources`, which digs to an empty object; and for any other non-map intermediate, current Helm turns the underlying sprig type assertion into a render error rather than a panic. ### Interaction with the firmware block `firmware` (UEFI/BIOS selection, Secure Boot, persistent EFI NVRAM) landed in this chart in the meantime, and it does not touch the sizing question. `domainResources` does not read it, so it takes no part in the completeness decision. KubeVirt's instancetype spec carries neither firmware nor `domain.features`, so nothing an instance type declares can conflict with what the firmware block renders. The boot mode a VM inherits when the block is empty comes from the preference, and the preference is not what gets dropped when complete resources replace the matcher. Both firmware states are covered on each side by the byte-identity comparison above. `README.md`, `values.schema.json`, `api/apps/v1alpha1/vminstance/types.go` and the ApplicationDefinition all come out of `make generate` in the package, not out of a hand merge. Re-running it on the result produces no further diff, and the root `make generate` reports no drift on top of it. ### On the ticket's second proposal #2909 also asks whether the non-empty `u1.medium` default belongs in the chart at all. That requirement is met, and not by removing the default. The default was a problem because a schema-generated form gave no way to say "no instance type": clearing the field falls back to `u1.medium`. Filling cpu, sockets and memory now says it directly, so the obstacle the ticket names is gone while the default stays. Dropping the default on top of that would change the rendered spec of every release that never set the field, with nothing left to gain. ### Named improvements, not done here The failure message names the fields that did not size the VM, computed from what was emitted, so for `cpu: 4` with no `sockets` it names `resources.cpu` even though the user set it. The message states the pair rule that explains this, but naming the actually-missing field first would read better. The two tests that pin `domain.cpu.cores: 0` document the pre-existing `int64` truncation in prose rather than through a `FIXME` reference, so a later reader has to read the comment to tell a contract from a bug we chose not to fix. Both are wording; neither changes behaviour. `resources.cpu`, `sockets` and `memory` are checked for shape and not for value, so `memory: "0"` and `-1Gi` pass the schema and fail further down. Both forms fail before and after this change and only the place of the failure moves, so that belongs with [#4104](https://github.com/cozystack/cozystack/issues/4104) rather than here. The new template comments run to about 46 lines against about 28 lines of logic. All of it is why rather than what and all of it is accurate, but the helper's third paragraph restates the commit body, and two sentences would carry that weight. The seven-line block in `vm.yaml` is the one worth keeping: swapping that gate to the effective value survives the entire suite, so what the block records is an equivalence no test can pin, and trimming it would delete the only record of it. The `instanceType` description runs to about 370 characters and reaches the dashboard as tooltip help, through `values.schema.json` and the ApplicationDefinition. That is long for a tooltip. It is accurate as it stands, and rewriting accurate text for length is a fresh unmeasured claim in the surface with the most readers, so it is not done here: the place for the detail about what a dropped instance type takes with it is the chart README, leaving the field help carrying the precedence rule alone. Choosing a `cx1.*` type and then giving it complete resources drops what the type guaranteed along with the type, and nothing says so at runtime. That is a consequence of the precedence rule rather than a defect in it, and the field help states it. A runtime signal would need the chart to compare what the type declares against what `resources` sets, which is its own task. Defaulting `sockets` to 1 would let a user size a VM from cpu and memory alone and would remove the commonest way to land in the partial case. It is more than the ticket asked for and it changes the rendered spec of stored releases that set cpu and memory without sockets, so it needs its own call on compatibility. ### Not changed Two pre-existing quirks stay as they are, because both reach past the conflict this fixes. `resources` fields are read for truthiness, so an explicit `0` reads as unset, in `domainResources` and in the existing "Either instanceType or resources" check alike. And with no instance type in play, a fractional `resources.cpu` still truncates to `cores: 0` and renders a VM sized by nothing, with no error. The new guard covers only the half where a matcher is present. The tests pin both as they stand, which is what keeps working releases rendering. ### Screenshots No dashboard code is touched. The only user-visible surface is two field descriptions the dashboard renders as help text, regenerated into `values.schema.json` and the ApplicationDefinition. `instanceType` now reads "Ignored when `resources` sizes the VM, which needs whole-number cpu and sockets plus memory; the whole type is dropped then, so any dedicated CPU placement, hugepages, NUMA or realtime settings it carries go with it. While this field is set, a `resources` block that sizes only part of the VM is rejected outright instead of ignored, so complete it or clear it." `resources` states the same rule from its own side. ### Downstream repositories - [ ] 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 and no listed trigger fires: no package added, renamed or removed; no `values.schema.json` field added, renamed or removed; no default changed; nothing under `hack/` moved; `templates/service.yaml` and its `wholeIP` and `allowICMP` annotations untouched. Two repositories do carry prose this makes incomplete. I left every box empty rather than push follow-ups nobody agreed to, so a maintainer decides. `cozystack/website`: `content/en/docs/next/virtualization/resources.md` opens with "Each virtual machine has these two configuration settings" and lists only `instanceType` and `instanceProfile`. The page is hand-written, so the release-time bot that regenerates app reference pages from `README.md` will not reach it. `cozystack/terraform-provider-cozystack`: `internal/provider/vminstance_schema.go` describes `instance_type` as "Virtual machine instance type." Its `kubernetes_schema.go` and `kubernetes_nodes_schema.go` already spell out the equivalent precedence rule for their own charts. No schema field or default changed here, so its new-field test guard has nothing to catch. ### Release note ```release-note fix(vm-instance): a complete `resources` block (whole-number cpu and sockets, plus memory) now sizes the VM directly and suppresses the `instanceType` matcher, instead of rendering both and having KubeVirt refuse the VM. Beside an `instanceType`, a `resources` block that sizes the VM only in part now stops the render with a message naming both sides, where it previously produced a VM the admission webhook rejected. Note that suppressing the matcher drops the whole instance type, including any dedicated CPU placement, hugepages, NUMA or realtime settings it carries. A release whose values already carry both converts on the next platform upgrade rather than at a moment the operator picks: the pre-upgrade hook strips `spec.instancetype` off the live VM and writes the domain sizing in its place, so plan that upgrade as if the VM restarts. ```
Cozystack
Cozystack is a free platform and framework for building clouds.
Cozystack is a CNCF Sandbox Level Project that was originally built and sponsored by Ænix.
With Cozystack, you can transform a bunch of servers into an intelligent system with a simple REST API for spawning Kubernetes clusters, Database-as-a-Service, virtual machines, load balancers, HTTP caching services, and other services with ease.
Use Cozystack to build your own cloud or provide a cost-effective development environment.
Use-Cases
-
Using Cozystack to build a public cloud
You can use Cozystack as a backend for a public cloud -
Using Cozystack to build a private cloud
You can use Cozystack as a platform to build a private cloud powered by Infrastructure-as-Code approach -
Using Cozystack as a Kubernetes distribution
You can use Cozystack as a Kubernetes distribution for Bare Metal
Documentation
The documentation is located on the cozystack.io website.
Read the Getting Started section for a quick start.
If you encounter any difficulties, start with the troubleshooting guide and work your way through the process that we've outlined.
Versioning
Versioning adheres to the Semantic Versioning principles.
A full list of the available releases is available in the GitHub repository's Release section.
Contributions
Contributions are highly appreciated and very welcomed!
In case of bugs, please check if the issue has already been opened by checking the GitHub Issues section. If it isn't, you can open a new one. A detailed report will help us replicate it, assess it, and work on a fix.
You can express your intention to on the fix on your own. Commits are used to generate the changelog, and their author will be referenced in it.
If you have a concrete feature request, open an issue. For a cross-cutting or architectural change, open a design proposal in the community repo. For questions or early ideas, use GitHub Discussions. See CONTRIBUTING.md for the full routing.
Community
You are welcome to join our Telegram group and come to our weekly community meetings. Add them to your Google Calendar or iCal for convenience.
License
Cozystack is licensed under Apache 2.0.
The code is provided as-is with no warranties.
Commercial Support
A list of companies providing commercial support for this project can be found on official site.