feat(operator): support shmSize on VLLMRuntime for tensor parallelism (#899) - #994
Conversation
The VLLMRuntime CRD had no way to enlarge /dev/shm, so tensor-parallel vLLM pods were stuck with the container default (typically 64Mi), which is too small for the shared-memory IPC that TP uses. The Helm chart can work around this with extra volumes, but the operator could not. Add a DeploymentConfig.shmSize field. When set, the controller mounts an emptyDir with medium=Memory at /dev/shm sized to the given quantity (e.g. "24Gi"); an unparseable value still yields a Memory-backed /dev/shm without a size limit rather than crashing the reconcile. Fixes vllm-project#899 Signed-off-by: Tai An <antai12232931@outlook.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new ShmSize configuration option to VLLMRuntime to allow mounting an emptyDir (medium=Memory) at /dev/shm for tensor parallelism. The feedback highlights a bug where updates to shmSize will not trigger a Deployment update because the controller does not compare volumes or volume mounts. Additionally, it is recommended to use *resource.Quantity instead of string to leverage Kubernetes API-level validation and prevent silent failures when parsing invalid quantity strings.
| 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 | ||
| } | ||
| volumes = append(volumes, corev1.Volume{Name: "dshm", VolumeSource: shmSource}) | ||
| volumeMounts = append(volumeMounts, corev1.VolumeMount{ | ||
| Name: "dshm", | ||
| MountPath: "/dev/shm", | ||
| }) | ||
| } |
There was a problem hiding this comment.
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
}| if q, err := resource.ParseQuantity(vllmRuntime.Spec.DeploymentConfig.ShmSize); err == nil { | ||
| shmSource.EmptyDir.SizeLimit = &q | ||
| } |
There was a problem hiding this comment.
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.
| // 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"` |
There was a problem hiding this comment.
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"`
}
|
Could you resolve the suggestions from gemini? |
Addresses review feedback on vllm-project#994: - deploymentNeedsUpdate now detects drift in the /dev/shm volume/mount so toggling shmSize on an existing VLLMRuntime actually rolls the Deployment. Compares the 'dshm' volume/mount specifically (not the whole Volumes slice) to avoid a reconcile loop from API-server defaulting on other volume sources such as the chat-template ConfigMap (DefaultMode). - Invalid shmSize quantities are no longer silently swallowed: log the error instead of mounting an unbounded /dev/shm with no indication. Kept ShmSize as a string for consistency with the existing cpu/gpu/memory resource fields, which are all plain strings in ResourceRequirements. Signed-off-by: Tai An <antai12232931@outlook.com>
71f2ada to
ebbac3e
Compare
Addresses review feedback on vllm-project#994: - deploymentNeedsUpdate now detects drift in the /dev/shm volume/mount so toggling shmSize on an existing VLLMRuntime actually rolls the Deployment. Compares the "dshm" volume/mount specifically (not the whole Volumes slice) to avoid a reconcile loop from API-server defaulting on other volume sources such as the chat-template ConfigMap (DefaultMode). - Invalid shmSize quantities are no longer silently swallowed: log the error instead of mounting an unbounded /dev/shm with no indication. Kept ShmSize as a string for consistency with the existing cpu/gpu/memory resource fields, which are all plain strings in ResourceRequirements. Signed-off-by: Tai An <antai12232931@outlook.com>
|
Thanks @ruizhang0101 — addressed the gemini review in cd3c1ee: High — shmSize changes not triggering a Deployment update: Medium — silent failure on invalid quantity: an unparseable Medium — |
| // Detect drift in the /dev/shm volume (driven by shmSize). Without this, | ||
| // toggling shmSize on an existing VLLMRuntime is seen as "no change" and | ||
| // the running Deployment never picks up (or drops) the mount. | ||
| // | ||
| // We compare the "dshm" volume/mount specifically rather than the whole | ||
| // Volumes slice: the API server defaults fields on some volume sources | ||
| // (e.g. ConfigMap DefaultMode, used by the chat-template volume) that the | ||
| // freshly generated expected spec does not carry, so a blanket | ||
| // reflect.DeepEqual would report a permanent mismatch and cause an endless | ||
| // reconcile loop. The dshm emptyDir/mount have no such server-side | ||
| // defaulting, so comparing them directly is safe. | ||
| if !reflect.DeepEqual( |
There was a problem hiding this comment.
Could you make the docstring concise?
|
Thanks @ruizhang0101 — condensed the drift-detection comment to 4 lines (kept the rationale for comparing the |
Signed-off-by: Tai An <antai12232931@outlook.com>
07e3f56 to
e85b10d
Compare
2 participants
What
Adds a
shmSizefield to theVLLMRuntimeCRD (deploymentConfig.shmSize). When set, the operator mounts anemptyDirwithmedium: Memoryat/dev/shm, sized to the given quantity.Fixes #899.
Why
Tensor-parallel vLLM communicates between ranks over shared memory. In containers
/dev/shmdefaults to a small size (typically 64Mi), which is too small for TP and leads to crashes /Bus error. The Helm chart can be worked around with extra volumes, but theVLLMRuntimeoperator had no equivalent knob — exactly what the issue asks for (shmSize: 24g).What changed
api/v1alpha1/vllmruntime_types.go— new optionalShmSize stringonDeploymentConfig.internal/controller/vllmruntime_controller.go— whenShmSize != "", append adshmemptyDir{medium: Memory}volume + a/dev/shmmount on the vLLM container. The size is parsed withresource.ParseQuantity; an unparseable value still yields a Memory-backed/dev/shm(no size limit) rather than panicking the reconcile loop.config/crd/bases/...vllmruntimes.yaml— regenerated CRD property.Behavior
dshmvolume is only added when requested).shmSize: "24Gi"→/dev/shmbacked by a 24Gi MemoryemptyDir.Example:
🤖 Generated with Claude Code