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
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,10 @@ Key values in `chart/values.yaml`:
| `worker.resources.requests.cpu` | `1` | Worker CPU request |
| `worker.resources.requests.memory` | `2Gi` | Worker memory request |
| `worker.runtimeClassName` | - | Runtime class for worker pods (e.g., `nvidia` for GPU) |
| `autoscaling.enabled` | `false` | Enable Ray in-tree autoscaling of worker pods |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not about this row, but this is the nearest line in the diff I can anchor to: the two pre-existing rows just above (worker.minReplicas / worker.maxReplicas, lines 217-218) are now misleading. Until this PR, "for autoscaling" pointed at a feature the chart could not turn on. Now they are the control surface, and their defaults are what make the new flag inert.

Suggested replacement for those two rows:

| `worker.minReplicas` | `1` | Min workers. Only meaningful with `autoscaling.enabled`; must be widened for autoscaling to do anything. |
| `worker.maxReplicas` | `1` | Max workers. Only meaningful with `autoscaling.enabled`; leave at `1` and the group never scales. |

🤖 claude-opus-5 (medium) · review-pr skill · reviewed by @oldsj

| `autoscaling.idleTimeoutSeconds` | `60` | Seconds an idle worker is kept before scale-down |
| `autoscaling.upscalingMode` | `Default` | `Default`, `Conservative`, or `Aggressive` |
| `autoscaling.resources` | `{}` | Optional resources for the autoscaler sidecar container |

### Serve Applications

Expand Down
2 changes: 1 addition & 1 deletion chart/Chart.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ apiVersion: v2
name: nebari-rayserve-pack
description: A Nebari Software Pack for Ray Serve
type: application
version: 0.4.1
version: 0.5.0
appVersion: "2.43.0"
dependencies:
- name: kuberay-operator
Expand Down
10 changes: 10 additions & 0 deletions chart/templates/rayservice.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,16 @@ spec:

rayClusterConfig:
rayVersion: {{ .Values.image.tag | quote }}
{{- if .Values.autoscaling.enabled }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth a maintainer call rather than a change request: should the template fail (or at least warn via NOTES.txt) when autoscaling.enabled is true and worker.maxReplicas is not greater than worker.minReplicas? As it stands that combination is silently accepted, costs 500m/512Mi, and delivers nothing. A hard fail may be too aggressive for a chart people --set interactively, but the current silence is the worst of the three options.

🤖 claude-opus-5 (medium) · review-pr skill · reviewed by @oldsj

enableInTreeAutoscaling: true
autoscalerOptions:
idleTimeoutSeconds: {{ .Values.autoscaling.idleTimeoutSeconds }}
upscalingMode: {{ .Values.autoscaling.upscalingMode }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, and the CRD enum catches the genuinely bad cases at admission. But the value interpolates unquoted, so --set autoscaling.upscalingMode=@foo fails as a Helm YAML parse error rather than anything legible, and upscalingMode: yes renders as a YAML boolean. | quote makes the failure land at CRD validation with a useful message instead.

Suggested change
upscalingMode: {{ .Values.autoscaling.upscalingMode }}
upscalingMode: {{ .Values.autoscaling.upscalingMode | quote }}

Deliberately not suggesting | int on idleTimeoutSeconds above — sprig's int turns 60s into 0, which would silently give you an autoscaler that reaps workers instantly. Unquoted is better there: a bad value reaches the API server and gets rejected on type.

🤖 claude-opus-5 (medium) · review-pr skill · reviewed by @oldsj

{{- with .Values.autoscaling.resources }}
resources:
{{- toYaml . | nindent 8 }}
{{- end }}
{{- end }}
headGroupSpec:
rayStartParams:
dashboard-host: "0.0.0.0"
Expand Down
46 changes: 46 additions & 0 deletions chart/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,52 @@ worker:
timeoutSeconds: 2
failureThreshold: 120

# =============================================================================
# Ray In-Tree Autoscaling
# =============================================================================
# Enable Ray's in-tree autoscaler to dynamically scale worker pods based on
# task demand. When `autoscaling.enabled` is true, the chart:
# - Sets `enableInTreeAutoscaling: true` on the RayCluster spec, causing
# KubeRay to attach an autoscaler sidecar container to the head pod.
# - Uses `worker.minReplicas` and `worker.maxReplicas` as the scaling range
# (both fields are already exposed above; without autoscaling enabled
# they are ignored by KubeRay).
#
# Without autoscaling.enabled the chart is byte-identical to the pre-existing
# behaviour: worker.replicas is the deployed count, min/max are inert.
#
# See the KubeRay autoscaling guide for background:
# https://docs.ray.io/en/latest/cluster/kubernetes/user-guides/configuring-autoscaling.html
#
# Ray's docs explicitly note that autoscaling "adds node launch overheads
# and can be tricky to configure" — starting with a fixed replica count
# (autoscaling.enabled: false) is recommended for newcomers.
Comment on lines +228 to +244

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The block explains what the flag does but not the two things that bite. First, worker.minReplicas and worker.maxReplicas both default to 1, so enabling this alone gives a 1..1 range and nothing ever scales — I confirmed the pin both in KubeRay's GetWorkerGroupDesiredReplicas clamp (utils/util.go:326-347) and live. Second, the sidecar is not free: pod.go:523-531 hardcodes requests and limits of 500m/512Mi, which I verified on a running head pod, so the head's scheduling footprint grows from 1/2Gi to 1.5/2.5Gi.

There is also a propagation trap worth recording here, since this block is where someone will look. The RayService controller strips replicas/minReplicas/maxReplicas from its spec-hash comparison (rayservice_controller.go:1098-1110) so the autoscaler can own them — which means a helm upgrade that changes only worker.maxReplicas never reaches the running cluster. I upgraded maxReplicas 4 → 1 and watched the RayService report max=1 while the live RayCluster stayed at max=4.

Suggested change
# Enable Ray's in-tree autoscaler to dynamically scale worker pods based on
# task demand. When `autoscaling.enabled` is true, the chart:
# - Sets `enableInTreeAutoscaling: true` on the RayCluster spec, causing
# KubeRay to attach an autoscaler sidecar container to the head pod.
# - Uses `worker.minReplicas` and `worker.maxReplicas` as the scaling range
# (both fields are already exposed above; without autoscaling enabled
# they are ignored by KubeRay).
#
# Without autoscaling.enabled the chart is byte-identical to the pre-existing
# behaviour: worker.replicas is the deployed count, min/max are inert.
#
# See the KubeRay autoscaling guide for background:
# https://docs.ray.io/en/latest/cluster/kubernetes/user-guides/configuring-autoscaling.html
#
# Ray's docs explicitly note that autoscaling "adds node launch overheads
# and can be tricky to configure" — starting with a fixed replica count
# (autoscaling.enabled: false) is recommended for newcomers.
# Enable Ray's in-tree autoscaler to dynamically scale worker pods based on
# task demand. When `autoscaling.enabled` is true, the chart:
# - Sets `enableInTreeAutoscaling: true` on the RayCluster spec, causing
# KubeRay to attach an autoscaler sidecar container to the head pod.
# - Uses `worker.minReplicas` and `worker.maxReplicas` as the scaling range
# (both fields are already exposed above; without autoscaling enabled
# they are ignored by KubeRay).
#
# You must widen the range yourself. Both bounds default to 1, so enabling
# autoscaling without also raising `worker.maxReplicas` pins the group at one
# worker and changes nothing.
#
# The sidecar is not free. KubeRay hardcodes its requests AND limits to
# 500m CPU / 512Mi memory unless `autoscaling.resources` overrides them, so
# the head pod's scheduling footprint grows by that much. With the chart's
# default head requests (1 CPU / 2Gi) the pod asks for 1.5 CPU / 2.5Gi.
#
# The scaling range is effectively install-time only. KubeRay deliberately
# excludes replicas/minReplicas/maxReplicas from the RayService spec hash so
# the autoscaler can own them, so a `helm upgrade` changing only
# worker.minReplicas/maxReplicas will not reach a running cluster. Changing
# any other rayClusterConfig field (including the autoscaler options below)
# triggers a full zero-downtime cluster replacement, which is what actually
# flushes a new range through.
#
# Without autoscaling.enabled the chart is byte-identical to the pre-existing
# behaviour: worker.replicas is the deployed count, min/max are inert.
#
# See the KubeRay autoscaling guide for background:
# https://docs.ray.io/en/latest/cluster/kubernetes/user-guides/configuring-autoscaling.html
#
# Ray's docs explicitly note that autoscaling "adds node launch overheads
# and can be tricky to configure" — starting with a fixed replica count
# (autoscaling.enabled: false) is recommended for newcomers.

🤖 claude-opus-5 (medium) · review-pr skill · reviewed by @oldsj

autoscaling:
enabled: false

# Time in seconds an idle worker pod is kept alive before being scaled
# down. Lower values reclaim resources faster but can create pod churn if
# tasks arrive in bursts.
idleTimeoutSeconds: 60

# upscalingMode:
# - `Default` — standard responsiveness
# - `Conservative` — pending requests scale up in batches on a delay
# - `Aggressive` — immediately upscale to satisfy demand
upscalingMode: Default
Comment on lines +253 to +257

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two of these three are inaccurate. From KubeRay v1.3.0 ray-operator/apis/ray/v1/raycluster_types.go:122-126 and the mapping in Ray 2.43.0 python/ray/autoscaler/_private/kuberay/autoscaling_config.py, Conservative sets upscaling_speed = 1 while both Default and Aggressive set 1000Aggressive is an alias, not a faster tier.

Suggested change
# upscalingMode:
# - `Default` — standard responsiveness
# - `Conservative` — pending requests scale up in batches on a delay
# - `Aggressive` — immediately upscale to satisfy demand
upscalingMode: Default
# upscalingMode — read by the Ray autoscaler, not the KubeRay operator:
# - `Default` — upscaling is not rate-limited
# - `Conservative` — rate-limited: pending worker pods are capped at the
# number of workers already connected to the cluster
# - `Aggressive` — an alias for `Default`; behaviour is identical
upscalingMode: Default

🤖 claude-opus-5 (medium) · review-pr skill · reviewed by @oldsj


# Optional overrides for the autoscaler sidecar container's resources.
# KubeRay's own defaults apply when unset ({}); typical production
# override:
# resources:
# limits:
# cpu: "500m"
# memory: "512Mi"
# requests:
# cpu: "500m"
# memory: "512Mi"
resources: {}

# =============================================================================
# Overrides
# =============================================================================
Expand Down
Loading