Skip to content

Commit fa2cedc

Browse files
authored
feat(kubernetes): let a node group set the CPU limit of its worker VMs (#3859)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does Adds `nodeGroups.<name>.podCpuLimit` to the kubernetes chart. It renders into `domain.resources.limits.cpu` on the worker VM, which is the CPU ceiling of the virt-launcher `compute` container. The field has no default, so a cluster that does not set it renders exactly what it rendered before. The knob exists because there is currently no way to give a worker VM a ceiling different from its vCPU count. `nodeGroups.<name>.resources.cpu` sizes the guest, not the pod. KubeVirt fills the pod side in: when the namespace carries a ResourceQuota that limits CPU, it sets `limits.cpu` to the guest vCPU count, and a tenant namespace with `resourceQuotas` set always does. So a worker sized at N vCPUs runs under a ceiling of N, and the QEMU emulator thread and the IO threads come out of the same N as the vCPU threads. Declaring the limit on the VM is the only clean way out: KubeVirt stops deriving one as soon as the VMI carries it (`doesVMIRequireAutoResourceLimits` returns early), and the value declared on the VM lands on the container. Removing `cpu` from the tenant's `resourceQuotas` instead would be worse, because the tenant LimitRange then applies its 250m default. The field moves the ceiling and nothing else. It renders no CPU request, so the request stays what KubeVirt computes from the vCPU count and the cluster CPU allocation ratio, and the scheduler weight of a worker does not change with it. That is deliberate: a knob that moved both would make its own effect unreadable. It cannot be combined with `instanceType` sizing, and the chart says so at render time rather than letting the failure land later. KubeVirt's instancetype applier reports `domain.resources.limits.cpu` as a conflict instead of resolving it, so a VM that carries both an instancetype and the limit is refused by the admission webhook, which leaves a node group with no Machine and a Kubernetes CR that explains nothing. Sizing a group by `resources` is exactly what drops the instancetype from the VM, so that is the combination the limit is usable in, and a group still sized by its `instanceType` now fails the render with a message naming the group. This sits beside the chart's existing guard for the neighbouring `resources.cpu` without `resources.memory` mistake, which is the same conflict class one field over. Rendering is byte-identical for a node group that sets nothing, down to the whitespace, and that is load-bearing rather than tidy. The `KubevirtMachineTemplate` around this block is named by a hash of its content, so a stray key or a moved space would rename the template for every node group in every cluster and roll every live worker VM on upgrade. The existing hash pins in `cluster_test.yaml` cover it: they stay green, and they go red under a mutation that renders the limit unconditionally. The e2e suite sets the limit to 3 for its two-vCPU workers. This is an experiment aimed at a measured mechanism, not a claimed fix for #3513. What a per-thread capture on a red run showed: the two vCPU threads of a stuck worker burn 1.6 to 1.8 cores between them under a ceiling of exactly 2.0 while the guest console records no progress at all, and the emulator, IO, vhost and vnc threads together stay under half a percent of the sampling interval. That is the shape of a vCPU spinning on a lock whose holder is the other vCPU, preempted out of the quota the two share, and it predicts that a ceiling with room above the vCPU count lets the preempted one back in. If the node-join failure rate does not move, the prediction was wrong and the mechanism needs a different lever; the chart field is useful either way, since a ceiling equal to the vCPU count is not a sensible default for a nested guest that pays emulation overhead out of it. Tests are helm-unittest in the same package: the limit lands in `domain.resources.limits.cpu`, a fractional quantity survives, no CPU request appears beside it, the ephemeral-storage pair the block already carried is untouched on both sides, nothing renders when the field is absent, and a group that keeps its instancetype fails the render on the message that names the reason. The `kubernetes-nodes` chart gets the same field, its guard and its own tests, because that chart is the other half of a byte-identity contract rather than a separate feature surface. Its `tests/render-parity.sh` renders a pool through both charts and compares the four pool objects byte for byte, on the grounds that the `KubevirtMachineTemplate` is content-hash named. A field only one side knows would not fail that gate, it would pass it and then quietly drop the ceiling when a pool is adopted into a split-out release. The gate gains a case that sets the limit and compares the two renders, and that case goes red if either side stops rendering it. ### 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. --> Not a 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. --> Walked against the diff. One repository is reached: `terraform-provider-cozystack` models every app field by hand, and a new field in `packages/apps/kubernetes/values.schema.json` needs the schema, the model and the expand/flatten pair over there, the same way `logSerialConsole` did in cozystack/terraform-provider-cozystack#26 and `kernelModules` did in cozystack/terraform-provider-cozystack#20. The follow-up is filed as cozystack/terraform-provider-cozystack#32. The website is not reached: its trigger is adding, renaming or removing a package, and the reference page for an existing package is regenerated from its `README.md` by the release bot. The remaining repositories restate nothing this diff touches. The change to `hack/e2e-chainsaw/_lib/run-kubernetes.sh` is a value inside the suite, not a move, a rename or a make target, so `ccp` is untouched. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: cozystack/terraform-provider-cozystack#32 - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note feat(kubernetes): a node group can now set `podCpuLimit`, the CPU ceiling of its worker VMs, independently of the vCPU count that sizes the guest. Without it KubeVirt keeps deriving that ceiling from the vCPU count, which leaves the QEMU emulator and IO threads sharing the guest's own allowance. Unset by default, so existing clusters render and behave exactly as before. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added optional `podCpuLimit` configuration for setting CPU limits on worker VM containers. * Supports integer and fractional Kubernetes CPU quantities. * Preserves CPU requests and ephemeral-storage settings while applying the configured limit. * Worker changes trigger VM template rollouts. * **Bug Fixes** * Added validation for incompatible combinations with instance-type sizing and insufficient resource configuration. * **Documentation** * Updated configuration references and schemas with usage, sizing, scheduling, and rollout guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents 9e3f66f + 78b9bc3 commit fa2cedc

18 files changed

Lines changed: 486 additions & 53 deletions

File tree

‎api/apps/v1alpha1/kubernetes/types.go‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎api/apps/v1alpha1/kubernetes/zz_generated.deepcopy.go‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎api/apps/v1alpha1/kubernetesnodes/types.go‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎api/apps/v1alpha1/kubernetesnodes/zz_generated.deepcopy.go‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎hack/e2e-chainsaw/_lib/run-kubernetes.sh‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3163,6 +3163,16 @@ ${ouroboros_addon}
31633163
logSerialConsole: true
31643164
maxReplicas: 10
31653165
minReplicas: 2
3166+
# Headroom over the vCPU count, where KubeVirt would otherwise put the
3167+
# ceiling. Workers failing to join (cozystack/cozystack#3513) have been
3168+
# measured burning their vCPU threads flat out at a ceiling equal to
3169+
# their vCPU count while the guest kernel made no progress, which is what
3170+
# a vCPU spinning on a lock looks like when the vCPU holding that lock is
3171+
# the one preempted out of the quota they share. Whether the spare CPU
3172+
# lets the preempted one back in is what this value is here to answer:
3173+
# measured mechanism, predicted remedy. If the join failure rate does not
3174+
# move, the lever is elsewhere and this goes back to the vCPU count.
3175+
podCpuLimit: 3
31663176
# Two vCPUs at the memory the workers had before. Sized by resources
31673177
# rather than by instanceType: u1.large doubles memory along with the
31683178
# vCPUs, and four 8Gi workers do not fit beside the rest of the suite,

0 commit comments

Comments
 (0)