Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Feature][Helm] Expose the autoscalerOptions #666

Merged
merged 2 commits into from
Nov 3, 2022
Merged

[Feature][Helm] Expose the autoscalerOptions #666

merged 2 commits into from
Nov 3, 2022

Conversation

orcahmlee
Copy link
Contributor

Why are these changes needed?

The options of the Autoscaler are useful but not exposed to the Helm values.yaml, such as idleTimeoutSeconds and resources.

Related issue number

Closes #665

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@orcahmlee
Copy link
Contributor Author

Hi @DmitriGekhtman ,
Would you mind reviewing this PR?

@DmitriGekhtman
Copy link
Collaborator

Deferring to @kevin85421 for this one.

@orcahmlee
[Feature][Helm] Added some example settings to values.yaml (ray-proje…
f4ad16a 
…ct#665)

The descriptions were copied from the `ray-cluster.autoscaler.large.yaml`

Signed-off-by: Andrew Li <orcahmlee@gmail.com>
@orcahmlee
Copy link
Contributor Author

orcahmlee commented Nov 2, 2022 •
edited

Hi @kevin85421 ,
I've merged the master and modified the commits. Please give me some feedbacks.

DmitriGekhtman
DmitriGekhtman reviewed Nov 2, 2022
View reviewed changes
helm-chart/ray-cluster/templates/raycluster-cluster.yaml Outdated
{{- if .Values.head.autoscalerOptions }}
{{- $values := .Values.head.autoscalerOptions}}
autoscalerOptions:
upscalingMode: {{ $values.upscalingMode | default "Default" }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

All of these fields are optional -- It's better not to set any defaults here. Let's leave decisions on defaults to the KubeRay operator.

kevin85421
kevin85421 reviewed Nov 2, 2022
View reviewed changes
helm-chart/ray-cluster/templates/raycluster-cluster.yaml Outdated
@@ -14,6 +14,17 @@ spec:
{{- if .Values.head.enableInTreeAutoscaling }}
enableInTreeAutoscaling: {{ .Values.head.enableInTreeAutoscaling }}
{{- end }}
{{- if .Values.head.autoscalerOptions }}
{{- $values := .Values.head.autoscalerOptions}}
autoscalerOptions:
Copy link
Member

Choose a reason for hiding this comment

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

I am not an expert in autoscaler, but I guess these options will take effect only when enableInTreeAutoscaling is true based on autoscaler.md (is this correct? cc @DmitriGekhtman).

If so, you need to take enableInTreeAutoscaling into consideration. For example, maybe you need to move the logic into the if statement of enableInTreeAutoscaling.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To match the behavior of RayCluster CR itself and accommodate future changes to autoscalerOptions,
I would just check if autoscalerOptions is specified in the chart and copy autoscalerOptions wholesale into the CR if it is present. In other words, I don't recommend analyzing subfields of autoscalerOptions in the chart.

Copy link
Member

Choose a reason for hiding this comment

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

Thank @DmitriGekhtman for your reply!

To match the behavior of RayCluster CR itself and accommodate future changes to autoscalerOptions

Q1: Can you talk more about why this modification will not match the behavior of RayCluster CR?

I don't recommend analyzing subfields of autoscalerOptions in the chart.

Q2: Do you mean that you recommend using toYaml to copy the whole autoscalerOptions into the CR as I said in the following review comment?

My thought is as follows

  1. Move autoscalerOptions into the if statement of enableInTreeAutoscaling
  2. Use toYaml to copy the whole autoscalerOptions.
{{- if .Values.head.enableInTreeAutoscaling }}
enableInTreeAutoscaling: {{ .Values.head.enableInTreeAutoscaling }}
{{- if .Values.head.autoscalerOptions }}
autoscalerOptions: {{- toYaml .Values.head.autoscalerOptions | nindent 4}}
{{- end }}
{{- end }}

Thank you!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean that you recommend using toYaml to copy the whole autoscalerOptions into the CR as I said in the following review comment?

Yes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Move autoscalerOptions into the if statement of enableInTreeAutoscaling
Use toYaml to copy the whole autoscalerOptions.

The net effect is the same. I recommend against, because the RayCluster CR allows setting autoscalerOptions with enableInTreeAutoscaling false. (Though the autoscalerOptions are useless in this situation.)

Copy link
Member

Choose a reason for hiding this comment

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

The net effect is the same. I recommend against, because the RayCluster CR allows setting autoscalerOptions with enableInTreeAutoscaling false. (Though the autoscalerOptions are useless in this situation.)

Got it. Thank you!

helm-chart/ray-cluster/templates/raycluster-cluster.yaml
{{- if $values.resources }}
resources: {{- toYaml $values.resources | nindent 6 }}
{{- end }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

In addition, it is unnecessary to expand all fields of autoscalerOptions in raycluster-cluster.yaml. You can use toYaml (example) to pass it from values.yaml to raycluster-cluster.yaml in one line.

autoscalerOptions: {{- toYaml .Values.head.autoscalerOptions | nindent 4}}

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

@orcahmlee
[Feature][Helm] Expose the autoscalerOptions to values.yaml (ray-proj…
918001d 
…ect#665)

Using toYaml instead of exposing all of the autoscalerOptions fileds.
This will leave the decisions on defaults to the KubeRay operator.

Signed-off-by: Andrew Li <orcahmlee@gmail.com>
@orcahmlee
Copy link
Contributor Author

Hi @DmitriGekhtman @kevin85421 ,
Thanks for your help. I did learn a lot from your discussions and this PR.
Your recommendation keeps not only the flexibility of autoscalerOptions but the template clear.

kevin85421
kevin85421 approved these changes Nov 3, 2022
View reviewed changes
Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

Thank @orcahmlee for the contribution! This PR looks good to me. If you have tested it manually, I will merge it.

@DmitriGekhtman DmitriGekhtman merged commit 4e9fdb0 into ray-project:master Nov 3, 2022
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
@orcahmlee
[Feature][Helm] Expose the autoscalerOptions (ray-project#666)
0daa290 
Expose autoscalerOptions in RayCluster Helm chart.

Signed-off-by: Andrew Li <orcahmlee@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevin85421 kevin85421 kevin85421 approved these changes

@DmitriGekhtman DmitriGekhtman DmitriGekhtman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature][Helm] Expose the autoscalerOptions
3 participants
@orcahmlee @DmitriGekhtman @kevin85421