[Feat][Helm] Support customizable resources and init containers for RayCluster#992
Conversation
…ay cluster Extends the Ray cluster template so users can now: - Provide a raw Kubernetes resources block on both the Ray head (raySpec.headNode.resources) and the Ray workers (modelSpec.resources). When set, the raw block takes priority over the simplified request*/limit* fields, mirroring the existing behavior in deployment-vllm-multi.yaml. - Attach an init container to the Ray head pod via raySpec.headNode.initContainer and to each Ray worker pod via modelSpec.initContainer, using the same block shape as the regular vLLM deployment (name, image, command, args, env, resources, mountPvcStorage, extraVolumeMounts). The values.yaml documentation, values.schema.json and helm/README.md are updated so contributors can discover the new fields, and helm/tests/ray-cluster_test.yaml is extended with helm-unittest cases that cover the new head/worker init containers, the raw resources overrides, and the fall-through to the simplified request* fields. Bumps helm/Chart.yaml to 0.1.12 per the chart's version-bump instructions. The change is backwards compatible: existing raySpec configurations without initContainer or resources render identically to before. Signed-off-by: a.alsheykh <a.a.alsheykh@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request adds support for deploying models as a multi-node RayCluster via KubeRay, including configurations for head and worker node init containers and raw resource overrides. The review feedback suggests making the Helm templates more robust by defensively checking for required fields (such as name and image in initContainer and name in envFromSecret) to prevent rendering invalid Kubernetes YAML, as well as simplifying the pvcMountEnabled truthiness checks.
| securityContext: | ||
| {{- toYaml . | nindent 10 }} | ||
| {{- end }} | ||
| {{- if and (hasKey $modelSpec "raySpec") (hasKey $modelSpec.raySpec "headNode") (hasKey $modelSpec.raySpec.headNode "initContainer") }} |
There was a problem hiding this comment.
If initContainer is defined but lacks a name or image (e.g., if it is defined as an empty map {}), Helm will render invalid Kubernetes YAML because name and image are required fields for containers. We should defensively check that both name and image are specified before rendering the initContainers block.
{{- if and (hasKey $modelSpec "raySpec") (hasKey $modelSpec.raySpec "headNode") (hasKey $modelSpec.raySpec.headNode "initContainer") $modelSpec.raySpec.headNode.initContainer.name $modelSpec.raySpec.headNode.initContainer.image }}| {{- if $container.args }} | ||
| args: {{ toYaml $container.args | nindent 14 }} | ||
| {{- end }} | ||
| {{- if $modelSpec.envFromSecret }} |
There was a problem hiding this comment.
| {{- if $container.resources }} | ||
| resources: {{ toYaml $container.resources | nindent 14 }} | ||
| {{- end }} | ||
| {{- $pvcMountEnabled := and (hasKey $container "mountPvcStorage") $container.mountPvcStorage (hasKey $modelSpec "pvcStorage") }} |
There was a problem hiding this comment.
We can simplify the $pvcMountEnabled calculation by directly checking the truthiness of the values instead of using hasKey multiple times. In Go templates, missing keys evaluate to nil (falsy), so this is completely safe and much cleaner.
{{- $pvcMountEnabled := and $container.mountPvcStorage $modelSpec.pvcStorage }}| securityContext: | ||
| {{- toYaml . | nindent 12 }} | ||
| {{- end }} | ||
| {{- if hasKey $modelSpec "initContainer" }} |
There was a problem hiding this comment.
Similarly to the head node, if the worker's initContainer is defined but lacks a name or image, it will render invalid Kubernetes YAML. We should defensively check that both name and image are specified before rendering the initContainers block.
{{- if and (hasKey $modelSpec "initContainer") $modelSpec.initContainer.name $modelSpec.initContainer.image }}| {{- if $container.args }} | ||
| args: {{ toYaml $container.args | nindent 16 }} | ||
| {{- end }} | ||
| {{- if $modelSpec.envFromSecret }} |
| {{- if $container.resources }} | ||
| resources: {{ toYaml $container.resources | nindent 16 }} | ||
| {{- end }} | ||
| {{- $pvcMountEnabled := and (hasKey $container "mountPvcStorage") $container.mountPvcStorage (hasKey $modelSpec "pvcStorage") }} |
Extends the Ray cluster template so users can now:
The values.yaml documentation, values.schema.json and helm/README.md are updated so contributors can discover the new fields, and helm/tests/ray-cluster_test.yaml is extended with helm-unittest cases that cover the new head/worker init containers, the raw resources overrides, and the fall-through to the simplified request* fields.
Bumps helm/Chart.yaml to 0.1.12 per the chart's version-bump instructions.
The change is backwards compatible: existing raySpec configurations without initContainer or resources render identically to before.
-swhen doinggit commit[Bugfix],[Feat], and[CI].