Skip to content

[INF-7324] (opt-in) hpa for jse deployment - #363

Draft
lukefoster11 wants to merge 4 commits into
mainfrom
lfoster/jse-hpa
Draft

[INF-7324] (opt-in) hpa for jse deployment#363
lukefoster11 wants to merge 4 commits into
mainfrom
lfoster/jse-hpa

Conversation

@lukefoster11

Copy link
Copy Markdown
Contributor

No description provided.

@greptile-apps

greptile-apps Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 4/5

The PR is not yet safe to merge because ContainerResource utilization metrics can still be validated against the wrong container.

The JS executor pod supports extra containers and passes custom HPA metrics through unchanged, but the new ContainerResource branch always checks rr.jsExecutor.resources.requests and ignores the container selected by the metric, allowing unavailable metrics or rejecting valid configurations.

Files Needing Attention: charts/retool/templates/deployment_js_executor.yaml

Important Files Changed

Filename Overview
charts/retool/templates/deployment_js_executor.yaml Adds the HPA and request validation, but ContainerResource validation checks the main container rather than the metric-selected container.
charts/retool/values.yaml Adds documented JS executor autoscaling defaults and accurately describes the general utilization-request dependency.
values.yaml Keeps the repository-level values copy synchronized with the deployable chart defaults.
charts/retool/ci/test-js-executor-hpa-option.yaml Exercises successful HPA rendering with shortcut and Pods metrics, though it does not cover ContainerResource validation.
charts/retool/Chart.yaml Increments the chart version for the feature release.

Reviews (3): Last reviewed commit: "version bump" | Re-trigger Greptile

Comment thread charts/retool/templates/deployment_js_executor.yaml
@lukefoster11 lukefoster11 changed the title (opt-in) hpa for jse deployment [INF-7324] (opt-in) hpa for jse deployment Aug 14, 2026
@lukefoster11

Copy link
Copy Markdown
Contributor Author

@greptile review me again

Comment thread charts/retool/templates/deployment_js_executor.yaml
@lukefoster11

Copy link
Copy Markdown
Contributor Author

@greptile review me again pls

Comment on lines +282 to +284
{{- if and (eq $targetType "Utilization") $name (not (index $requests $name)) }}
{{- fail (printf "rr.jsExecutor.autoscaling.metrics ContainerResource Utilization target %q requires rr.jsExecutor.resources.requests.%s. Utilization is unavailable without that request, so the HPA cannot scale on it. Set the request or use an AverageValue target instead." $name $name) }}
{{- end }}

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.

P1 Wrong container request validated

When a ContainerResource Utilization metric targets an extra container, this branch checks rr.jsExecutor.resources.requests for the main JS executor instead of the selected container. Helm therefore accepts a metric whose target lacks the request, leaving that HPA metric unavailable, or rejects a valid metric when only the target container has the request.

Knowledge Base Used:

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