Skip to content

[Feat][Helm] Support customizable resources and init containers for RayCluster#992

Open
TheCodingSheikh wants to merge 1 commit into
vllm-project:mainfrom
TheCodingSheikh:feat/helm-raycluster-init-container-and-resources
Open

[Feat][Helm] Support customizable resources and init containers for RayCluster#992
TheCodingSheikh wants to merge 1 commit into
vllm-project:mainfrom
TheCodingSheikh:feat/helm-raycluster-init-container-and-resources

Conversation

@TheCodingSheikh

Copy link
Copy Markdown

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.

  • Make sure the code changes pass the pre-commit checks.
  • Sign-off your commit by using -s when doing git commit
  • Try to classify PRs for easy understanding of the type of changes, such as [Bugfix], [Feat], and [CI].

…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>

@gemini-code-assist gemini-code-assist Bot left a comment

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.

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") }}

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

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 }}

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

If envFromSecret is defined but does not contain a name key, rendering will produce an empty name field under secretRef, which is invalid. We should defensively check that envFromSecret.name is defined.

            {{- if and $modelSpec.envFromSecret $modelSpec.envFromSecret.name }}

{{- if $container.resources }}
resources: {{ toYaml $container.resources | nindent 14 }}
{{- end }}
{{- $pvcMountEnabled := and (hasKey $container "mountPvcStorage") $container.mountPvcStorage (hasKey $modelSpec "pvcStorage") }}

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

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" }}

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

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 }}

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

Defensively check that envFromSecret.name is defined before rendering the envFrom block for the worker node's initContainer.

              {{- if and $modelSpec.envFromSecret $modelSpec.envFromSecret.name }}

{{- if $container.resources }}
resources: {{ toYaml $container.resources | nindent 16 }}
{{- end }}
{{- $pvcMountEnabled := and (hasKey $container "mountPvcStorage") $container.mountPvcStorage (hasKey $modelSpec "pvcStorage") }}

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

Simplify the $pvcMountEnabled calculation for the worker node's initContainer by directly checking the truthiness of the values.

              {{- $pvcMountEnabled := and $container.mountPvcStorage $modelSpec.pvcStorage }}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant