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
7 changes: 7 additions & 0 deletions operator/api/v1alpha1/vllmruntime_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@ type DeploymentConfig struct {
// +kubebuilder:default=nvidia
RuntimeClass string `json:"runtimeClass,omitempty"`

// ShmSize, when set, mounts an emptyDir with medium=Memory at /dev/shm
// sized to this value (e.g. "24Gi"). Tensor parallelism uses shared
// memory for inter-process communication and the container default
// /dev/shm (typically 64Mi) is too small. Accepts any Kubernetes quantity.
// +optional
ShmSize string `json:"shmSize,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.

medium

Recommendation: Use *resource.Quantity instead of string

Using a raw string for ShmSize requires manual parsing in the controller and can lead to silent failures or runtime errors if the user inputs an invalid quantity (e.g., "24Gii").

By changing the type to *resource.Quantity (from k8s.io/apimachinery/pkg/api/resource), the Kubernetes API server will automatically validate the field value upon creation or update, rejecting any invalid quantities before they ever reach the operator.

Example

import resource "k8s.io/apimachinery/pkg/api/resource"

// ...

type DeploymentConfig struct {
    // ...
    
    // ShmSize, when set, mounts an emptyDir with medium=Memory at /dev/shm
    // sized to this value (e.g. "24Gi").
    // +optional
    ShmSize *resource.Quantity `json:"shmSize,omitempty"`
}


// Resource requirements
Resources ResourceRequirements `json:"resources"`

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,13 @@ spec:
default: nvidia
description: RuntimeClass
type: string
shmSize:
description: |-
ShmSize, when set, mounts an emptyDir with medium=Memory at /dev/shm
sized to this value (e.g. "24Gi"). Tensor parallelism uses shared
memory for inter-process communication and the container default
/dev/shm (typically 64Mi) is too small. Accepts any Kubernetes quantity.
type: string
sidecarConfig:
description: Sidecar configuration
properties:
Expand Down
17 changes: 17 additions & 0 deletions operator/internal/controller/vllmruntime_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -738,6 +738,23 @@ func (r *VLLMRuntimeReconciler) deploymentForVLLMRuntime(
})
}

// Mount an emptyDir (medium=Memory) at /dev/shm when a size is requested.
// Tensor parallelism communicates over shared memory, and the container
// default /dev/shm is usually too small for it.
if vllmRuntime.Spec.DeploymentConfig.ShmSize != "" {
shmSource := corev1.VolumeSource{
EmptyDir: &corev1.EmptyDirVolumeSource{Medium: corev1.StorageMediumMemory},
}
if q, err := resource.ParseQuantity(vllmRuntime.Spec.DeploymentConfig.ShmSize); err == nil {
shmSource.EmptyDir.SizeLimit = &q
}
Comment on lines +748 to +750

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.

medium

Improvement: Silent failure on invalid shmSize quantity

If the user provides an invalid quantity string (e.g., "24Gii"), resource.ParseQuantity will fail, and the error is silently ignored. This results in an emptyDir being mounted with no size limit (which defaults to using the node's memory/tmpfs limits), without any indication to the user or administrator that their configuration was invalid.

Recommendation

Consider logging a warning when parsing fails, or refactoring deploymentForVLLMRuntime to return an error so that the reconciliation loop can fail/requeue and surface the invalid configuration in the resource status.

Alternatively, you can change the type of ShmSize in the API definition to *resource.Quantity (see the other comment) to leverage Kubernetes API-level validation and prevent invalid values from being accepted in the first place.

volumes = append(volumes, corev1.Volume{Name: "dshm", VolumeSource: shmSource})
volumeMounts = append(volumeMounts, corev1.VolumeMount{
Name: "dshm",
MountPath: "/dev/shm",
})
}
Comment on lines +744 to +756

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.

high

Bug: Changes to shmSize will not trigger a Deployment update

The deploymentNeedsUpdate function (lines 951-1108) does not compare the Pod's Volumes or VolumeMounts when determining if the existing Deployment needs to be updated.

As a result, if a user updates the shmSize field on an existing VLLMRuntime custom resource, the operator will detect no changes, and the Deployment will not be updated to mount the /dev/shm volume.

Recommendation

Update deploymentNeedsUpdate to compare the volumes and volume mounts of the expected and actual deployments. For example:

// Compare volumes
if !reflect.DeepEqual(expectedDep.Spec.Template.Spec.Volumes, dep.Spec.Template.Spec.Volumes) {
    log.Info("Volumes mismatch")
    return true
}

// Compare volume mounts for the main container
if !reflect.DeepEqual(expectedDep.Spec.Template.Spec.Containers[0].VolumeMounts, dep.Spec.Template.Spec.Containers[0].VolumeMounts) {
    log.Info("Volume mounts mismatch")
    return true
}


var affinity *corev1.Affinity

if vllmRuntime.Spec.DeploymentConfig.NodeSelectorTerms != nil {
Expand Down
Loading