Skip to content

feat(chart): add opt-in enableInTreeAutoscaling support - #27

Open
oren-openteams wants to merge 4 commits into
nebari-dev:mainfrom
oren-openteams:feat/enable-in-tree-autoscaling
Open

feat(chart): add opt-in enableInTreeAutoscaling support#27
oren-openteams wants to merge 4 commits into
nebari-dev:mainfrom
oren-openteams:feat/enable-in-tree-autoscaling

Conversation

@oren-openteams

Copy link
Copy Markdown
Collaborator

Summary

Adds an opt-in autoscaling.enabled value that wires Ray's in-tree autoscaler into the RayCluster spec. Default is off — chart output is byte-identical to today's behaviour for existing consumers.

The gap this closes

The chart already exposes worker.minReplicas and worker.maxReplicas, and the README values table labels both "for autoscaling". But the RayCluster template never emits enableInTreeAutoscaling: true, so KubeRay ignores those fields and always deploys exactly worker.replicas count of workers. The result is that setting minReplicas / maxReplicas has no observable effect today.

This PR wires the missing top-level flag through a new autoscaling: values block. When autoscaling.enabled: true:

  • Chart renders enableInTreeAutoscaling: true on the RayCluster spec
  • KubeRay attaches an autoscaler sidecar to the head pod
  • The already-existing worker.minReplicas and worker.maxReplicas become the scaling range
  • autoscalerOptions.idleTimeoutSeconds and .upscalingMode are configurable via the new block (defaults per Ray docs: 60 s and Default)

Changes

  • chart/values.yaml — new top-level autoscaling: block with enabled, idleTimeoutSeconds, upscalingMode, resources. All defaults preserve today's static-scaling behaviour.
  • chart/templates/rayservice.yaml — conditional enableInTreeAutoscaling: true + autoscalerOptions block, gated on .Values.autoscaling.enabled.
  • chart/Chart.yaml — minor bump 0.4.1 → 0.5.0 for the new feature.
  • README.md — four new rows in the values table documenting the block.

Backwards compatibility

Default autoscaling.enabled: false. Rendered manifests for any existing values file are unchanged — the new conditional emits nothing when disabled.

Real-world motivation

We hit this while sizing a Ray cluster for a Nebari deployment. The user needed 3+ CPUs on the Ray cluster (checkmaite reserves CPUs for its registry + per-job controller actors before any worker task can be placed). Static bump of worker.replicas unblocked them, but keeping 4 worker pods running 24/7 is wasteful when the cluster is idle. Autoscaling with minReplicas: 1, maxReplicas: 6 would let workers scale to zero-of-what's-needed when the cluster is quiet and up to 6 during active use.

Test plan

  • Deploy chart with autoscaling.enabled: false (default) → rendered RayService is byte-identical to pre-PR output. Confirmed via helm template diff.
  • Deploy chart with autoscaling.enabled: true, worker.minReplicas: 1, worker.maxReplicas: 6 → RayCluster spec has enableInTreeAutoscaling: true + autoscalerOptions; head pod gets the autoscaler sidecar container; worker count scales up when tasks are submitted and back down after idleTimeoutSeconds.
  • Override autoscaling.resources → sidecar container resources reflect the override.

References

@oldsj oldsj left a comment

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 template is correct and the opt-in shape is right — I confirmed against the vendored KubeRay 1.3.0 CRD that every field name and the upscalingMode enum are valid, and a live install on my dev cluster scaled 1 → 3 workers on demand and back down after idleTimeoutSeconds. The autoscaler sidecar attaches, and KubeRay creates its RBAC on its own, so nothing else is needed in the chart. I also reproduced the backwards-compatibility claim: with autoscaling.enabled: false the rendered output against origin/main differs only in the helm.sh/chart version label.

Four things to fix before this merges, all of them about what surrounds the template rather than the template itself.

1. This needs a rebase — main now says the opposite of what the PR does. The branch is based on 224834b, and main has since landed #32 and #33, replacing Docusaurus with an Astro/Starlight site and writing real content into it. That content asserts in three places that this feature does not exist: docs/src/content/docs/scaling.md:44-47 ("The chart does not set enableInTreeAutoscaling, so KubeRay runs no Ray autoscaler sidecar"), scaling.md:209-210 (autoscaling listed under "What is not here"), and configuration.md:52-57, whose whole :::caution block is premised on "there is no Ray autoscaler". The values reference in configuration.md also documents every chart value and would be missing autoscaling.* entirely. Updating README.md alone was sufficient when this branch was written; it no longer is.

2. autoscaling.enabled: true on its own does nothing, and costs 500m/512Mi to do it. worker.minReplicas and worker.maxReplicas both default to 1, so the scaling range is 1..1 and the group is pinned — confirmed in KubeRay's GetWorkerGroupDesiredReplicas clamp and in Ray's autoscaling_config.py, which maps the group to min_workers == max_workers == 1. Meanwhile KubeRay injects the autoscaler sidecar with hardcoded requests and limits of 500m CPU / 512Mi memory (pod.go:523-531, and I confirmed it on a live pod), so the head pod's scheduling footprint goes from the chart's default 1 CPU / 2Gi to 1.5 CPU / 2.5Gi. Flipping the flag alone is therefore a pure regression: more resources, identical behaviour, head pod possibly unschedulable on a tight node — and for a RayService a Pending head means the service is down. Both the sidecar overhead and the "you must also widen worker.maxReplicas" requirement need to be stated where someone will see them.

3. The scaling range can only be set at install time. This is the part I'd most want documented, because it is genuinely surprising. The RayService controller deliberately strips replicas, minReplicas, and maxReplicas out of the spec hash it compares against the live RayCluster, so the autoscaler can own them. The side effect is that a helm upgrade changing only worker.maxReplicas never reaches the cluster. I watched it: I upgraded maxReplicas from 4 to 1, the RayService spec dutifully showed max=1, and the live RayCluster sat at max=4 indefinitely. The range only updates as a side effect of some other rayClusterConfig field changing — and when that happens you get a full zero-downtime cluster replacement, not an in-place edit. Changing autoscaling.idleTimeoutSeconds from 60 to 90 spun up a second RayCluster and reaped the first, restarting head and all workers. So tuning any autoscaler option on a live inference service costs a cluster rebuild. Worth a sentence in scaling.md.

4. The upscalingMode comment in values.yaml is wrong on two of three modes. Per raycluster_types.go:122-126 and the mapping in Ray's autoscaling_config.py: Aggressive is a literal alias for Default (both set upscaling_speed = 1000), not a faster tier; and Conservative is not "batches on a delay" — it sets upscaling_speed = 1, capping pending worker pods at the number currently connected.

One smaller thing: enabling this on an ArgoCD-managed cluster after the initial sync will silently not apply. enableInTreeAutoscaling renders under spec.rayClusterConfig, which is exactly the path the documented Application ignores under RespectIgnoreDifferences=true — the same failure class already written up for nebari-rayserve-pack#17. Worth mirroring that warning for this feature.

Nothing here needs a template change except the optional quoting nit below. Rebase, move the docs, and I think this is good.


Verification actually run, for the record: helm template diff of origin/main vs this head with default values (identical but for the chart label); field names and the upscalingMode enum checked against the vendored ray.io_rayservices.yaml CRD; and a live install on an isolated namespace of my jamesolds-dev EKS cluster covering sidecar attachment, scale-up via request_resources, scale-down after the idle timeout, sidecar resource inspection, auto-created RBAC, the maxReplicas propagation test, and the zero-downtime cluster replacement. Not covered: Ray Serve's own per-deployment autoscaling_config interaction.

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

Comment thread chart/values.yaml
Comment on lines +228 to +244
# 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.

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

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

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

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


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

Comment thread README.md
| `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

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.

feat(chart): add opt-in enableInTreeAutoscaling support

2 participants