Repository navigation
docs(gpu): add a GPU overview page that maps the five paths - #704
Andrei Kvapil (kvaps) wants to merge 2 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
✅ Deploy Preview for cozystack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM. Three statements on the page don't match the code at either version, and two commit bodies would land in main as they are. Links are fine: I built the site at 2ce8018 and all seven targets resolve in both next/ and v1.6/.
I checked the next/ copy against cozystack main (9620be01) and the v1.6/ copy against v1.6.4. Line numbers are the same in both files, and every finding below applies to both copies.
Blockers
B1: the variant is chosen per cluster, not per node
The page says the choice is made "once per node" (:8), the heading at :22 says "A management cluster node serves one variant", and :30 says "Two of them on one node make a broken node". In the code the variant is one value for the whole management cluster. bundles.iaas.gpuOperatorVariant becomes a single $gpuVariant in packages/core/platform/templates/bundles/iaas.yaml (:63 on main, :36 on v1.6.4). It goes into the one Package CR named cozystack.gpu-operator (:212 on main, :160 on v1.6.4), and every variant in sources/gpu-operator.yaml installs the same gpu-operator release into cozy-gpu-operator.
So two variants can't meet on one node, because the platform never runs two variants. The real constraint goes the other way: switching to container turns VFIO management off on every GPU node at once. A reader planning container GPU nodes next to passthrough nodes in one cluster will read this page as saying that works.
Fix: say the variant is set once per management cluster and covers every GPU node in it. The host-ownership part can stay as the reason it is expensive to change.
B2: the "Selected through" column misses the switch that installs anything
Setting the variant alone installs nothing. Rows :15 and :16 give bundles.iaas.gpuOperatorVariant: default / vgpu, but the Package is rendered only when cozystack.gpu-operator is in bundles.enabledPackages and not in disabledPackages (iaas.yaml :62 and :211 on main, :35 and :159 on v1.6.4). enabledPackages defaults to [], and default is already the default variant (values.yaml:60). A reader who "selects" passthrough from this table changes nothing.
The next/ container row (:14) also lost the reason the PR body gives for it. Since website#714 (2900c01, merged 2026-10-06) the next/ container guide uses bundles.iaas.gpuOperatorVariant: container plus cozystack.gpu-operator in enabledPackages. Leaving the key out now makes the map disagree with that guide. The v1.6/ row is correct as written, because v1.6.4 rejects container in the bundle (iaas.yaml:44).
Fix: for the bundle rows in both copies, name cozystack.gpu-operator in bundles.enabledPackages next to the variant. In next/, give the container row the bundle key too. The #4101 paragraph in the PR body is no longer true and the body becomes the merge commit message, so please drop it.
B3: a hand edit to the KubeVirt CR survives the next reconcile
The page says at :32 that "a hand edit to the live resource is reverted on the next reconcile". It isn't. Operator-generated HelmReleases run with drift detection off, and helm-controller does not re-render a release whose chart and values haven't changed. The repo says this itself in internal/backupcontroller/default_objects_gate.go:110-116, same lines on main and v1.6.4: "a plain reconcile request does not re-render".
What actually happens is worse for the reader. The edit works until the next upgrade of the kubevirt release (any platform values change or version bump), and then it silently disappears. virtualization/gpu.md:121 has the same sentence in both versions; the map shouldn't copy it.
Fix: something like "a hand edit to the live resource is overwritten the next time the platform upgrades the kubevirt release".
B4: two commit bodies describe earlier drafts of the page
This repo merges with merge commits, so every commit body ends up in main. 300c898 says the paragraph "no longer promises a starter device table" and that the MIG item "now says only what holds everywhere". 2ce8018 says the link text "said a tenant cluster "certifies"" and "now says which requirements are met". Neither earlier text was ever on main, so these read as review-iteration notes, which the contributing rules don't accept in commit bodies. Please fold both into 8d6b6d8; the two merges from main can go away in the same rebase.
Non-blocking
- At :26 the
defaultvariant "unbinds whatever holds the card", while :30 and the host-driver troubleshooting page say it refuses when a host NVIDIA driver holds it. "Bindsvfio-pcito the card" avoids contradicting the next paragraph. - At :38 Kueue is listed as something a tenant cluster "offers". The AI Conformance page records Kueue v0.19.4 as something a tenant installs as an ordinary workload, and DRA as an API group with no driver shipped. "What a tenant can run on it" matches that page better.
- Outside this diff,
virtualization/gpu.md:121in both versions has the same "reverted on the next Flux/Helm reconcile" claim as B3.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) NOT LGTM. The variant scope, enabling switch and KubeVirt upgrade wording are fixed. These points remain:
- The glibc warning needs checking against the shipped version. In each new
guides/gpu.md:48, it still says HAMi relies on_dl_symand loses isolation on recent images. The shipped HAMi 2.9.0 source pins HAMi-core 5091a2fb, which includes the fix removing that dependency. Please remove the old blanket warning or replace it with a limitation verified for that version. I haven't tested GPU isolation on hardware. - A DLS endpoint is not a requirement for vGPU. The vGPU 20.0 licensing guide supports CLS or DLS. At
guides/gpu.md:46in each copy, say NVIDIA License System with CLS or DLS, rather than requiring a DLS deployment. - The history fix from my previous review is still missing. Commit
300c8984says "no longer promises" and "now says";2ce80181describes what the link text "said" and "now says". Please fold these into the original logical commit or rewrite their bodies to explain the reason without describing earlier drafts. - The PR body still describes the old page. It says the variant is picked "once per node", omits the v1.7 copy, and says the next container guide still uses the old selection method. It also says the vGPU guide omits the device-manager condition, although that condition is now explicit at
virtualization/vgpu.md:117. Please update the body to match the current files.
GPU documentation is spread over five pages in four sections, and nothing tells a reader which one applies to them. The gpu-operator variant is one platform value applied to every GPU node of the management cluster, and switching it means reworking the hosts, so a reader who starts on the wrong page pays for it later. The page maps each consumer to its path and its enabling switch, explains why the three operator variants exclude each other, shows how tenant clusters stack on passthrough, and lists the limits worth knowing before planning: MIG and VMs, vGPU licensing, and HAMi over the container variant. Tenant AI capabilities are left to the AI Conformance page, which is linked rather than paraphrased, and the link text names the requirements met without claiming a mark the project does not hold. No procedure is restated, so the page does not go stale when the guides it points to change. It is added to next/, v1.7 and v1.6. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Operator-generated HelmReleases run without drift detection, so a plain reconcile does not re-render the kubevirt release. A hand edit to permittedHostDevices survives until the release is next upgraded. Assisted-by: LLM Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
ed2ba90 to
1f54a60
Compare
|
Thanks, fixed. History is now two commits on top of main, and the description matches the current files. The glibc table and the DLS-only prerequisite are still on the HAMi and vGPU pages themselves. I listed them in the description and would rather fix them in a separate PR. |
GPU docs are spread over five pages in four sections, and nothing tells a reader which one they need. Here it costs more than usual: the gpu-operator variant is one platform value for every GPU node of the management cluster, and switching it means reworking the hosts.
This adds one map page under Learn Cozystack, in
next/,v1.7/andv1.6/. For each consumer it gives the path, the switch that enables it (the variant pluscozystack.gpu-operatorinbundles.enabledPackages) and the guide to read. Procedures stay in the guides, so the map doesn't need updates when they change. DRA, Kueue and DCGM in tenant clusters are left to the AI Conformance page, the map only links it.In
next/andv1.7/the container row usesbundles.iaas.gpuOperatorVariant: container, same as the container guide. The v1.6 bundle doesn't acceptcontainer, so there the row says a hand-written Package.Also a one-line fix in the passthrough guide, all three versions: a hand edit to
permittedHostDevicesin the KubeVirt CR is not reverted by a plain reconcile, since operator-generated HelmReleases run without drift detection. It is lost on the next upgrade of the kubevirt release.Left out on purpose, these need a real GPU node to check:
gpu=onlabel,NvLinkDisable)Stale on the linked pages, not touched here:
_dl_symanymore. The map doesn't carry that warningspec.nodeGroupson the Kubernetes app, but worker pools moved to KubernetesNodesChecked:
hugobuilds. Nothing tested on GPU hardware.