Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 95 additions & 0 deletions api/v1alpha1/clusterpolicy_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ type ClusterPolicySpec struct {
// +kubebuilder:validation:Range=0:4
// +kubebuilder:validation:Default=2
LogLevel int32 `json:"logLevel,omitempty"`

// KernelModule configures out-of-tree kernel module loading via KMM.
// When set, KMM loads the specified OOT driver module on each node.
// When nil, the in-tree kernel driver is used.
// +optional
KernelModule *KernelModuleSpec `json:"kernelModule,omitempty"`
}

// DynamicResourceAllocationSpec defines the desired state of DynamicResourceAllocation.
Expand Down Expand Up @@ -147,11 +153,99 @@ type XpuManagerSpec struct {
MonitoringResource string `json:"monitoringResource,omitempty"`
}

// RegistryTLSSpec configures TLS behavior for accessing container image registries.
type RegistryTLSSpec struct {
Insecure bool `json:"insecure,omitempty"`
InsecureSkipTLSVerify bool `json:"insecureSkipTLSVerify,omitempty"`
}

// KernelModuleSpec configures out-of-tree kernel module loading via KMM.
type KernelModuleSpec struct {
Comment thread
tkatila marked this conversation as resolved.
// ModuleName is the kernel module to load (defaults to "xe").
// +kubebuilder:default=xe
ModuleName string `json:"moduleName,omitempty"`

Comment thread
tkatila marked this conversation as resolved.
// Version is passed to the KMM container spec to trigger module updates.
// +optional
Version string `json:"version,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried setting this, but it seemed to stop any Module handling by kmmo. I couldn't find any obvious errors.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the Version field kicks off KMM's ordered upgrade process, which requires a cluster admin to set special labels on nodes so they can control upgrade rollout timing and minimize workload disruptions. E.g., when Version is set to "v0.0.2", KMM won't load the module into a node unless kmm.node.kubernetes.io/version-module.<module-namespace>.<module-name>=v0.0.2 is set on the node. This is opposed to just changing the value of the containerImage field, which will briefly disrupt GPU workloads on all selected nodes roughly simultaneously as the driver reloads.

Based on my past life as an SRE, I can see some cluster admins greatly appreciating the fine-grained rolling upgrade control that Version provides. I can also see other users becoming confused by it though, as it's admittedly not clear from the current docstrings that they should avoid Version entirely if they'd prefer the simpler all-at-once upgrade experience provided by changing containerImage.

Let's discuss. I see three options forward:

# Option Pros Cons
1 Improve docs on when to use Version/ordered upgrades vs. containerImage, otherwise leave code as-is Keeps a fine-grained rolling-upgrade capability for admins who want it and a simpler all-at-once option for everyone else Relies on users reading docs
2 Remove Version/ordered-upgrade support; only support all-at-once upgrades via containerImage Simplifies and slims CRD even further; still offers an update trigger that works with zero admin choreography Loses node-by-node sequencing, i.e., driver rolls across all selected nodes near-simultaneously; brief GPU-workload disruption fleet-wide
3 Add machinery to auto-manage the version-module label (rolling sequencer, GPU workload rescheduling, node join/leave handling) Offers users a low-disruption "just works" experience Lots of new, stateful code and some new RBAC needed; increased bug blast radius

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. option is the most interesting one as an engineer, but I think the first option is the best. While users tend to ignore documentation, by giving versionless examples we can avoid most of the confusion. But still leave the logic for advanced users.


// KernelMappings maps kernel version patterns to container images or
// build specifications. Translates directly to KMM KernelMapping objects.
// +kubebuilder:validation:MinItems=1
KernelMappings []KernelMappingSpec `json:"kernelMappings"`

Comment thread
tkatila marked this conversation as resolved.
// ModulesLoadingOrder specifies softdep-style loading order for
// multi-module drivers. First element must be ModuleName (defaults
// to "xe"); KMM loads in order and unloads in reverse. Must have
// >=2 entries if set.
// +optional
ModulesLoadingOrder []string `json:"modulesLoadingOrder,omitempty"`

Comment thread
tkatila marked this conversation as resolved.
// FirmwarePath is the in-container path where firmware files are stored.
// +optional
FirmwarePath string `json:"firmwarePath,omitempty"`
Comment thread
tkatila marked this conversation as resolved.

// RegistryTLS configures TLS for accessing the module image registry.
// +optional
RegistryTLS *RegistryTLSSpec `json:"registryTLS,omitempty"`
}

// KernelMappingSpec maps a kernel version pattern to a container image
// or build specification.
type KernelMappingSpec struct {
// Regexp is a regular expression matched against node kernel versions.
// Use anchored patterns (e.g. "^5\\.14\\.0-.*$") for exact matches.
Regexp string `json:"regexp"`

// ContainerImage is the full image reference for this kernel version.
// Required when Build is nil. KMM template vars (e.g. ${KERNEL_FULL_VERSION},
// $MOD_NAME) are supported and resolved by KMM at reconcile time.
// +optional
ContainerImage string `json:"containerImage,omitempty"`

// Build configures in-cluster building of the driver image via KMM.
// When set, KMM builds the image if it doesn't exist in the registry.
// +optional
Build *KernelModuleBuildSpec `json:"build,omitempty"`

Comment thread
tkatila marked this conversation as resolved.
// InTreeModulesToRemove lists additional in-tree modules to unload
// for this mapping. ModuleName is always included automatically.
// +optional
InTreeModulesToRemove []string `json:"inTreeModulesToRemove,omitempty"`

// RegistryTLS overrides parent-level TLS settings for this mapping.
// +optional
RegistryTLS *RegistryTLSSpec `json:"registryTLS,omitempty"`
Comment on lines +216 to +218

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? Or do we need the parent-level TLS?

}

// KernelModuleBuildSpec configures in-cluster driver image building.
type KernelModuleBuildSpec struct {
// DockerfileConfigMap references a ConfigMap containing the Dockerfile.
DockerfileConfigMap v1.LocalObjectReference `json:"dockerfileConfigMap"`

// BuildArgs are key-value pairs passed to the image builder.
// +optional
BuildArgs []BuildArg `json:"buildArgs,omitempty"`

// Secrets are made available during the build (e.g., for private
// source repos). Not for registry auth -- use pullSecret on
// ClusterPolicySpec.
// +optional
Secrets []v1.LocalObjectReference `json:"secrets,omitempty"`
}

// BuildArg is a key-value pair passed as a build argument.
type BuildArg struct {
Name string `json:"name"`
Value string `json:"value"`
}

// ClusterPolicyStatus defines the observed state of ClusterPolicy.
type ClusterPolicyStatus struct {
DevicePluginStatus string `json:"devicePluginStatus,omitempty"`
DRAStatus string `json:"draStatus,omitempty"`
XPUManagerStatus string `json:"xpuManagerStatus,omitempty"`
KMMStatus string `json:"kmmStatus,omitempty"`
Errors []string `json:"errors,omitempty"`
}

Expand Down Expand Up @@ -183,6 +277,7 @@ type LocalQueueSpec struct {
// +kubebuilder:printcolumn:name="DP",type=string,JSONPath=`.status.devicePluginStatus`
// +kubebuilder:printcolumn:name="DRA",type=string,JSONPath=`.status.draStatus`
// +kubebuilder:printcolumn:name="XPU",type=string,JSONPath=`.status.xpuManagerStatus`
// +kubebuilder:printcolumn:name="KMM",type=string,JSONPath=`.status.kmmStatus`
// +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`
// +operator-sdk:csv:customresourcedefinitions:displayName="Intel GPU Cluster Policy"
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
Expand Down
103 changes: 103 additions & 0 deletions api/v1alpha1/clusterpolicy_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ func (d *ClusterPolicyCustomDefaulter) Default(_ context.Context, cp *ClusterPol
spec.XpuManagerSpec.MonitoringResource = "monitoring"
}

if spec.KernelModule != nil && spec.KernelModule.ModuleName == "" {
spec.KernelModule.ModuleName = "xe"
}

return nil
}

Expand All @@ -95,6 +99,7 @@ func validateClusterPolicySpec(spec *ClusterPolicySpec) (admission.Warnings, err
errs = append(errs, validatePullSecret(spec)...)
errs = append(errs, validateConfigMapOverride(spec)...)
errs = append(errs, validateKueueSpec(spec)...)
errs = append(errs, validateKernelModuleSpec(spec)...)

if w := warnForSpecProblems(spec); w != "" {
warnings = append(warnings, w)
Expand Down Expand Up @@ -223,6 +228,104 @@ func validateKueueSpec(spec *ClusterPolicySpec) []error {
return errs
}

func validateKernelModuleSpec(spec *ClusterPolicySpec) []error {
if spec.KernelModule == nil {
return nil
}

km := spec.KernelModule
var errs []error

for i, m := range km.KernelMappings {
prefix := fmt.Sprintf("kernelModule.kernelMappings[%d]", i)
errs = append(errs, validateKernelMapping(prefix, &m)...)
}

if len(km.ModulesLoadingOrder) > 0 {
errs = append(errs, validateModulesLoadingOrder(km.ModuleName, km.ModulesLoadingOrder)...)
}

return errs
}

// kmmTemplateVarRegexp matches KMM's ${VAR} and $VAR template placeholders
// (e.g. ${KERNEL_FULL_VERSION}, $MOD_NAME), which KMM resolves at reconcile time.
var kmmTemplateVarRegexp = regexp.MustCompile(`\$\{[^}]*\}|\$[A-Za-z_][A-Za-z0-9_]*`)

// substituteKMMTemplateVars swaps KMM template placeholders for a tag-safe token
// so the reference can be validated as a normal image reference.
func substituteKMMTemplateVars(image string) string {
return kmmTemplateVarRegexp.ReplaceAllString(image, "0")
}

func validateImageTagOrDigest(field, image string) error {
// Validate a copy with template vars resolved; report the original on error.
resolved := substituteKMMTemplateVars(image)

if _, err := reference.ParseNormalizedNamed(resolved); err != nil {
return fmt.Errorf("invalid image reference in %s: %q: %w", field, image, err)
}

if !strings.Contains(resolved, ":") && !strings.Contains(resolved, "@") {
return fmt.Errorf("%s: image %q must include an explicit tag or digest", field, image)
}

return nil
}

func validateKernelMapping(prefix string, m *KernelMappingSpec) []error {
var errs []error

if m.Regexp == "" {
errs = append(errs, fmt.Errorf("%s: regexp is required", prefix))
} else {
if _, err := regexp.Compile(m.Regexp); err != nil {
errs = append(errs, fmt.Errorf("%s.regexp: invalid regular expression: %w", prefix, err))
}
}

if m.ContainerImage != "" {
if err := validateImageTagOrDigest(prefix+".containerImage", m.ContainerImage); err != nil {
Comment thread
tkatila marked this conversation as resolved.
errs = append(errs, err)
}
}

if m.ContainerImage == "" && m.Build == nil {
errs = append(errs, fmt.Errorf("%s: one of containerImage or build must be set", prefix))
}

if m.Build != nil {
if m.Build.DockerfileConfigMap.Name == "" {
errs = append(errs, fmt.Errorf("%s.build.dockerfileConfigMap.name is required", prefix))
}
}

return errs
}

func validateModulesLoadingOrder(moduleName string, order []string) []error {
var errs []error

if len(order) < 2 {
errs = append(errs, fmt.Errorf("kernelModule.modulesLoadingOrder must have at least 2 entries"))
}

if len(order) > 0 && moduleName != "" && order[0] != moduleName {
errs = append(errs, fmt.Errorf("kernelModule.modulesLoadingOrder[0] must be moduleName %q", moduleName))
}

seen := make(map[string]bool, len(order))
for i, entry := range order {
if seen[entry] {
errs = append(errs, fmt.Errorf("kernelModule.modulesLoadingOrder[%d]: duplicate entry %q", i, entry))
break
}
seen[entry] = true
}

return errs
}

// warnForSpecProblems returns a warning message if some old or deprecated option is set.
func warnForSpecProblems(spec *ClusterPolicySpec) string {
if spec.ResourceRegistration == "dp" && spec.DevicePluginSpec.LevelzeroImage != "" {
Expand Down
Loading