Add optional KMM integration for OoT kernel module loading - #79
Conversation
72f70ab to
f6b068d
Compare
| // inserting the OOT module. ModuleName is always included implicitly. | ||
| // +optional | ||
| InTreeModulesToRemove []string `json:"inTreeModulesToRemove,omitempty"` | ||
|
|
There was a problem hiding this comment.
Is the 'InTree' prefix necessary here? Would there be cases in the future that the out-of-tree kernel module should be removed instead?
There was a problem hiding this comment.
InTreeModulesToRemove matches the field name that KMM's Module CR uses. For each $MODULE_NAME passed in InTreeModulesToRemove, KMM checks if /lib/modules/.../$MODULE_NAME.ko exists (which is usually only true for non-KMM-installed modules, i.e., in-tree modules or modules installed via DKMS) before running modprobe -r $MODULE_NAME. KMM only removes KMM-installed OoT KMDs when their Module CR is deleted or when the node no longer matches the Module's node selector. One could argue that "PreExisting" or "OnNode" or "NonKMM" might be better at illustrating this nuance than "InTree", but doing so here would break upstream precedent and potentially confuse users.
So yes, I think the "InTree" prefix makes sense here. It matches established convention and signals to users that KMM-managed OoT modules shouldn't be listed here (they'd be silently skipped). I'm open to discussion though :)
| // Literal is an exact kernel version string to match. | ||
| // Mutually exclusive with Regexp. | ||
| // +optional | ||
| Literal string `json:"literal,omitempty"` |
There was a problem hiding this comment.
Couldn't this be covered by Regexp above, which is just a string, e.g. "6.18.44"? Shouldn't we let the controller figure this out instead of having two fields for the same thing, especially since we have the "fallback" Image specified one step above?
There was a problem hiding this comment.
It certainly can, and seeing as we're aiming to slim down the CRD, it shall 🙂. I'll drop Literal in favor of Regexp, and users can just do Regexp = "^6\\.18\\.44$" for exact-match situations
| // When set, KMM builds the image if it doesn't exist in the registry. | ||
| // +optional | ||
| Build *KernelModuleBuildSpec `json:"build,omitempty"` | ||
|
|
There was a problem hiding this comment.
Once the system starts building, do we know if it succeeded and ready for use in order to proceed with any next steps dependent on the kernel module - or is this relevant for the use case?
There was a problem hiding this comment.
We surface KMM's module loader status (available/desired) in ClusterPolicy.Status.KMMStatus for observability. For sequencing, I'll modify the DP/DRA/XPU Manager controllers to wait for KMM's kmm.node.kubernetes.io/{ns}.{name}.ready node label like @tkatila suggested. That label only appears once the module is successfully loaded, so pods won't schedule on nodes where the build/load hasn't completed.
| // in order and unloads in reverse. Must have >=2 entries if set. | ||
| // +optional | ||
| ModulesLoadingOrder []string `json:"modulesLoadingOrder,omitempty"` | ||
|
|
There was a problem hiding this comment.
Do we repeat the 'xe' ModuleName in this array? Shouldn't this only contain dependencies for ModuleName?
There was a problem hiding this comment.
KMM requires ModuleName as the first entry in this list, followed by its dependencies in order. I think it increases clarity to force users to explicitly declare the full ordering here (including xe/ModuleName), and that way we can just directly pass this field through to KMM. I'm open to discussion though :)
|
|
||
| // FirmwarePath is the in-container path where firmware files are stored. | ||
| // +optional | ||
| FirmwarePath string `json:"firmwarePath,omitempty"` |
There was a problem hiding this comment.
There is no /usr/lib/firmware where to always look for the firmware(s)?
There was a problem hiding this comment.
It depends on how the driver container image is built, and KMM (as we're just mirroring KMM's API here) doesn't set a default value here because an empty value means "no firmware to copy," which is the right default for modules that don't ship firmware
| // for mappings that omit ContainerImage. | ||
| // +optional | ||
| Image string `json:"image,omitempty"` | ||
|
|
There was a problem hiding this comment.
Since we have ContainerImage in KerneMappings, should this be the DefaultImage instead? Or should KernelMappings be made mandatory with at least one entry instead, now there seems to be a bit of growth in information duplication. Whatever fine-grained information the kmm system needs, this operator can surely patch together?
tkatila
left a comment
There was a problem hiding this comment.
Some comments related to the CRD changes. I think the Module CR is somewhat complex as it has same fields in different places and would like to not copy them as they are. If possible.
| type KernelModuleSpec struct { | ||
| // ModuleName is the kernel module to load (e.g., "xe"). | ||
| // Also used as the default InTreeModulesToRemove entry. | ||
| ModuleName string `json:"moduleName"` |
There was a problem hiding this comment.
This should default to 'xe'. We don't have plans to support the OoT i915 driver. Variable could even be completely removed.
There was a problem hiding this comment.
Noted! Will add the default value
| // SkipTLSVerify disables TLS certificate verification when pulling | ||
| // OOT driver images. For air-gapped or internal registries. | ||
| // +optional | ||
| SkipTLSVerify bool `json:"skipTLSVerify,omitempty"` |
There was a problem hiding this comment.
Let's use the registryTLS object from the Module instead. insecure and insecureSkipTLSVerify can be used in both container.registryTLS and container.kernelMappings.registryTLS
There was a problem hiding this comment.
Agreed. I'll replace SkipTLSVerify with a RegistryTLS struct matching KMM's TLSOptions shape (insecure + insecureSkipTLSVerify) and allow it to be set both at the top level and per-mapping.
| } | ||
|
|
||
| // KernelModuleSpec configures out-of-tree kernel module loading via KMM. | ||
| type KernelModuleSpec struct { |
There was a problem hiding this comment.
I think we need a version entry in the KernelModule. Xe OoT KMD has tagged versions and I think it could serve as a trigger for KMD updates?
There was a problem hiding this comment.
Good call. KMM's ModuleLoaderContainerSpec has a Version field for exactly this purpose. Will add it to our struct and map it through
| // When KernelMappings is non-empty, serves as the KMM-level fallback | ||
| // for mappings that omit ContainerImage. | ||
| // +optional | ||
| Image string `json:"image,omitempty"` |
There was a problem hiding this comment.
Is using this same as having .* in the kernelMappings? If so, I think this can be dropped. KMDs are always tied to specific kernel versions, so it's not possible to have a generic container for many kernels.
There was a problem hiding this comment.
It is indeed, and good point that it's not very useful for version-specific KMDs. I'll drop Image entirely and make KernelMappings required
| // Literal is an exact kernel version string to match. | ||
| // Mutually exclusive with Regexp. | ||
| // +optional | ||
| Literal string `json:"literal,omitempty"` |
There was a problem hiding this comment.
Needed if we just use Regexp?
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| func generateNodeSelector(cp *v1alpha.ClusterPolicy) map[string]string { |
There was a problem hiding this comment.
Shouldn't the KMM's node labels be used if the KernelModule is enabled? i.e. only deploy Pods when the KMD is loaded?
| // InTreeModulesToRemove lists in-tree modules to unload before | ||
| // inserting the OOT module. ModuleName is always included implicitly. | ||
| // +optional | ||
| InTreeModulesToRemove []string `json:"inTreeModulesToRemove,omitempty"` |
There was a problem hiding this comment.
Can this be also removed as it's included in the kernelMappings object?
There was a problem hiding this comment.
Sure, I'll drop the top-level InTreeModulesToRemove and keep the the per-mapping version (for cases where specific kernel versions need extra modules removed). I'll also make it such that gpu-base-operator always tells KMM to remove any existing in-tree modules named "xe" (or whatever ModuleName is set to), so users never need to specify it manually.
988d5fc to
c2de30e
Compare
Add KMMReconciler sub-controller that creates a KMM Module CR when the new ClusterPolicy.spec.kernelModule field is set and KMM is installed. Native DP/DRA controllers are unchanged -- KMM is used only for out-of-tree kernel module management (modprobe, kernel mappings, in-cluster builds). Includes webhook validation, OpenShift SCC/RBAC support via Helm, and controller/webhook tests. Signed-off-by: Anthony Byrne <abyrne@redhat.com>
- Extract anyAllocatedResourceClaims to controller_utils.go so DRA and KMM share one implementation - Call updateStatus before the error return so KMMStatus reflects current module state even when CreateOrPatch fails - Deep-copy Secrets slice in convertBuildSpec to avoid sharing the backing array Signed-off-by: Anthony Byrne <abyrne@redhat.com>
Report module loader availability mismatches, stuck deletions, and ResourceClaim-blocked deletions in ClusterPolicy status errors. Signed-off-by: Anthony Byrne <abyrne@redhat.com>
c2de30e to
a85526a
Compare
|
Thanks @abyrne55 for the changes! I'll try to use this on my end in the next couple of days. |
Rework the KMM integration API based on review feedback: - Default moduleName to "xe" via CRD schema default and mutating webhook - Add version field mapping to KMM container.Version - Drop top-level image field; require kernelMappings (MinItems=1) - Replace skipTLSVerify bool with *RegistryTLSSpec (Insecure + InsecureSkipTLSVerify) - Drop literal field; require regexp on each kernel mapping - Gate downstream DaemonSets (DP, DRA, XPU) on KMM ready node label - Auto-set inTreeModulesToRemove from moduleName at container and per-mapping level - Pass modulesLoadingOrder through as-is - Clear stale Status.Errors each reconcile so it reflects current state Signed-off-by: Anthony Byrne <abyrne@redhat.com>
a85526a to
8e0e555
Compare
This PR adds a
KMMReconcilersub-controller that creates a KMMModuleCR when the newClusterPolicy.spec.kernelModulefield is set and KMM is installed. The existing DP and DRA controllers keep their lifecycle logic. KMM handles only out-of-tree kernel module loading (modprobe, kernel mappings, in-cluster builds).How it works
When
ClusterPolicy.spec.kernelModuleis set and KMM is installed in the cluster,KMMReconcilercreates a KMMModuleCR with amoduleLoaderspec. The Module CR is owned by the ClusterPolicy and garbage-collected on deletion. A deletion guard skips Module CR removal while GPU ResourceClaims are still allocated.KMM availability is detected at startup via API group discovery (
kmm.sigs.x-k8s.io), same pattern asDRAEnable. IfkernelModuleis set but KMM isn't installed, the operator reports an error in ClusterPolicy status. IfkernelModuleis nil, the sub-controller is a no-op.Downstream DaemonSets (DP, DRA, XPU Manager) are gated on the KMM ready node label (
kmm.node.kubernetes.io/<ns>.<module>.ready), so they only schedule once the OoT module is actually loaded on a node. WhenkernelModuleunset, however, their behavior is unchanged.Relationship to PR #59
#59 proposed replacing the native DP/DRA controllers entirely with KMM. Two controllers collapse into one, and gpu-base-operator stops managing DP/DRA DaemonSets, RBAC, SCCs, and DeviceClasses at runtime. That gives the biggest code reduction (~-3,100 lines) but makes KMM a hard dependency for all users.
This PR takes a more conservative approach: the native controllers stay in place for DP/DRA lifecycle, and a new
KMMReconcilerruns alongside them solely for OoT kernel module management. KMM is never required, and clusters using in-tree drivers work exactly as before. The trade-off is more total code (+2,456 / -51 lines across 30 files).kernelModuleAPIkernelMappingsmust contain at least 1 entry. Minimal usage: one OoT driver image matched to all kernels:More realistic usage: different images per kernel version
In-cluster build mode: useful when driver source is available but pre-built images don't exist for every kernel version in the fleet. KMM checks the target registry first and only triggers a build if the image is missing.
The
dockerfileConfigMapreferences a ConfigMap containing the Dockerfile.buildArgsare passed as build arguments, andsecretsare mounted during the build (e.g., for private source repos). Registry auth should usepullSecreton ClusterPolicySpec instead.Fields
moduleNamexe.versionkernelMappings[].regexpkernelMappings[].containerImagekernelMappings[].inTreeModulesToRemovemoduleNameis prepended automatically.inTreeModulesToRemove(container level)[moduleName]by the controller — the in-tree module is always unloaded before OoT insertion.modulesLoadingOrdermoduleName, >=2 entries). Passed through as-is.firmwarePathModprobeSpec.FirmwarePath).registryTLSinsecure,insecureSkipTLSVerify) for the OoT image registry. Settable at top level and per mapping.What's included
KernelModuleSpec/KernelMappingSpec/RegistryTLSSpecCRD types with DeepCopyKMMReconcilersub-controller withCreateOrPatch, owner references, and deletion guardmoduleNamedefaulting forkernelModulemodulesverbs) in the operator ClusterRolekernelModuleconfigurationTest results
Latest end-to-end run on OCP 4.22.9 SNO, 2× Intel BMG-G31 (Battlemage, PCI
8086:e223), kernel5.14.0-687.35.1.el9_8.x86_64, KMM v2.7.0 + NFD pre-installed:xe(kmmStatus: 1/1); ready label gated DP/DRA/XPU until load; DRA driver + XPU Manager1/1; ResourceSlice populated with both GPUs; DRA isolation test passed (/dev/dri/present in claimed container, absent otherwise); status errors cleared once healthymoduleNamedefaults toxe;regexprequired (webhook-enforced)Corroborating results from earlier runs (OCP 4.22.2, Arc Pro B70 with SR-IOV VFs):
kmmStatus: N/A, clean deletiongpu.intel.com/xe=8registered, GPU workload OKKnown limitations
xecan't unload with active VFs.modprobe -rvcannot unloadxewhile SR-IOV VFs are active. KMM is adding modprobe.d support (kubernetes-sigs/kernel-module-management#1324) that could work around this in the future.This PR was written in part with the assistance of generative AI.