[Operator] Add separate resource requests and limits - #1079
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for independent CPU and memory requests and limits within ResourceRequirements across VLLMRuntime, VLLMRouter, and CacheServer resources, while preserving backward compatibility with flat resource fields. It adds schema validation, helper functions for building and comparing resources, and comprehensive unit tests. One critical issue was identified in the sidecar container defaulting logic, where explicitly configured requests can exceed the default limits, potentially causing Pod creation failures. A fix was suggested to dynamically adjust limits to match requests in these scenarios.
| sidecarResourceConfig := sidecarConfig.Resources | ||
| if sidecarResourceConfig.CPU == "" { | ||
| sidecarResourceConfig.CPU = "0.5" | ||
| } | ||
|
|
||
| if sidecarConfig.Resources.CPU != "" { | ||
| sidecarResources.Requests[corev1.ResourceCPU] = resource.MustParse( | ||
| sidecarConfig.Resources.CPU, | ||
| ) | ||
| sidecarResources.Limits[corev1.ResourceCPU] = resource.MustParse( | ||
| sidecarConfig.Resources.CPU, | ||
| ) | ||
| } else { | ||
| sidecarResources.Requests[corev1.ResourceCPU] = resource.MustParse("0.5") | ||
| sidecarResources.Limits[corev1.ResourceCPU] = resource.MustParse("0.5") | ||
| if sidecarResourceConfig.Memory == "" { | ||
| sidecarResourceConfig.Memory = "128Mi" | ||
| } | ||
|
|
||
| if sidecarConfig.Resources.Memory != "" { | ||
| sidecarResources.Requests[corev1.ResourceMemory] = resource.MustParse( | ||
| sidecarConfig.Resources.Memory, | ||
| ) | ||
| sidecarResources.Limits[corev1.ResourceMemory] = resource.MustParse( | ||
| sidecarConfig.Resources.Memory, | ||
| ) | ||
| } else { | ||
| sidecarResources.Requests[corev1.ResourceMemory] = resource.MustParse("128Mi") | ||
| sidecarResources.Limits[corev1.ResourceMemory] = resource.MustParse("128Mi") | ||
| if sidecarResourceConfig.GPU == "" { | ||
| sidecarResourceConfig.GPU = "0" | ||
| } | ||
|
|
||
| if sidecarConfig.Resources.GPU != "" { | ||
| gpuType := "nvidia.com/gpu" | ||
| if sidecarConfig.Resources.GPUType != "" { | ||
| gpuType = sidecarConfig.Resources.GPUType | ||
| } | ||
| gpuResource := resource.MustParse(sidecarConfig.Resources.GPU) | ||
| sidecarResources.Requests[corev1.ResourceName(gpuType)] = gpuResource | ||
| sidecarResources.Limits[corev1.ResourceName(gpuType)] = gpuResource | ||
| } else { | ||
| gpuType := "nvidia.com/gpu" | ||
| if sidecarConfig.Resources.GPUType != "" { | ||
| gpuType = sidecarConfig.Resources.GPUType | ||
| } | ||
| zeroQty := resource.MustParse("0") | ||
| sidecarResources.Requests[corev1.ResourceName(gpuType)] = zeroQty | ||
| sidecarResources.Limits[corev1.ResourceName(gpuType)] = zeroQty | ||
| sidecarResources, err := buildResourceRequirements(sidecarResourceConfig) | ||
| if err != nil { | ||
| return corev1.Container{}, fmt.Errorf("build VLLMRuntime sidecar resources: %w", err) | ||
| } |
There was a problem hiding this comment.
When only requests.cpu or requests.memory is explicitly configured for the sidecar (e.g., requests.cpu: "1"), the flat defaults (cpu: "0.5" and memory: "128Mi") are still applied to the limits. This results in an invalid resource configuration where the request is greater than the limit (e.g., request 1 vs limit 0.5), causing the Kubernetes API server to reject the Pod creation.
To prevent this, we should ensure that the limits are automatically adjusted to be at least equal to the requests if the default limits are lower than the explicitly requested values.
sidecarResourceConfig := sidecarConfig.Resources
if sidecarResourceConfig.CPU == "" {
sidecarResourceConfig.CPU = "0.5"
}
if sidecarResourceConfig.Memory == "" {
sidecarResourceConfig.Memory = "128Mi"
}
if sidecarResourceConfig.GPU == "" {
sidecarResourceConfig.GPU = "0"
}
sidecarResources, err := buildResourceRequirements(sidecarResourceConfig)
if err != nil {
return corev1.Container{}, fmt.Errorf("build VLLMRuntime sidecar resources: %w", err)
}
// Ensure requests do not exceed limits due to sidecar defaulting
if cpuReq, ok := sidecarResources.Requests[corev1.ResourceCPU]; ok {
if cpuLimit, ok := sidecarResources.Limits[corev1.ResourceCPU]; ok {
if cpuReq.Cmp(cpuLimit) > 0 {
sidecarResources.Limits[corev1.ResourceCPU] = cpuReq.DeepCopy()
}
}
}
if memReq, ok := sidecarResources.Requests[corev1.ResourceMemory]; ok {
if memLimit, ok := sidecarResources.Limits[corev1.ResourceMemory]; ok {
if memReq.Cmp(memLimit) > 0 {
sidecarResources.Limits[corev1.ResourceMemory] = memReq.DeepCopy()
}
}
}There was a problem hiding this comment.
fixed in this update. sidecar defaults now adjust around one-sided CPU and memory values, and explicit request-over-limit combinations fail before reconciliation. added regressions for all four controller paths.
Signed-off-by: Michael Basov <124149774+banlor@users.noreply.github.com>
d940503 to
41a47fd
Compare
Summary
The operator currently copies one CPU and memory value into both requests and limits, so users cannot tune scheduling and caps independently.
Tests
go vet ./...go build ./...main; both had the same seven Windows envtest failures, and the e2e target requires GNU Make on this hostCloses #901