-
Notifications
You must be signed in to change notification settings - Fork 763
Feat/remove restart policy never if autoscaler v2 and ray 2.55 or greater #4816
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.
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
base: master
Are you sure you want to change the base?
Changes from 4 commits
7cad015
31cf894
715c260
797624a
5978fd1
0937f50
6be130a
2f8947f
a309e2a
f3458fd
565d9a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -213,9 +213,13 @@ func DefaultHeadPodTemplate(ctx context.Context, instance rayv1.RayCluster, head | |
| mergeAutoscalerOverrides(&autoscalerContainer, instance.Spec.AutoscalerOptions) | ||
| podTemplate.Spec.Containers = append(podTemplate.Spec.Containers, autoscalerContainer) | ||
|
|
||
| // The error is ignored here because the function will return false if there's an error parsing the version. | ||
| // For example, if rayVersion is empty or unparseable, it considers the feature is not valid. | ||
| if utils.IsAutoscalingV2Enabled(&instance.Spec) { | ||
| setAutoscalerV2EnvVars(&podTemplate) | ||
| podTemplate.Spec.RestartPolicy = corev1.RestartPolicyNever | ||
| if autoscalerRestartValid, _ := utils.IsRayVersionAtLeast(instance.Spec.RayVersion, utils.MinAutoscalerRestartValidVersion); !autoscalerRestartValid { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error from
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed at 6be130a along with the similar one in |
||
| podTemplate.Spec.RestartPolicy = corev1.RestartPolicyNever | ||
| } | ||
| } else if utils.IsAutoscalingV1Enabled(&instance.Spec) { | ||
| setAutoscalerV1EnvVars(&podTemplate) | ||
| } | ||
|
|
@@ -466,7 +470,11 @@ func DefaultWorkerPodTemplate(ctx context.Context, instance rayv1.RayCluster, wo | |
| podTemplate.Spec.Containers[utils.RayContainerIndex].Ports = append(podTemplate.Spec.Containers[utils.RayContainerIndex].Ports, metricsPort) | ||
| } | ||
|
|
||
| if utils.IsAutoscalingEnabled(&instance.Spec) && utils.IsAutoscalingV2Enabled(&instance.Spec) { | ||
| // Use the headGroupSpec to determine whether the RestartPolicy should be Never or not, since the head pod is the one that runs the autoscaler. | ||
| // The error is ignored here because the function will return false if there's an error parsing the version. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed at 565d9a8 along with the above. |
||
| // For example, if rayVersion is empty or unparseable, it considers the feature is not valid. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment says "Use the headGroupSpec" but the code actually reads from the cluster-level spec (
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed at 5978fd1 |
||
| autoscalerRestartValid, _ := utils.IsRayVersionAtLeast(instance.Spec.RayVersion, utils.MinAutoscalerRestartValidVersion) | ||
| if !autoscalerRestartValid && utils.IsAutoscalingEnabled(&instance.Spec) && utils.IsAutoscalingV2Enabled(&instance.Spec) { | ||
| podTemplate.Spec.RestartPolicy = corev1.RestartPolicyNever | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we keep
errhere, i think we should also re-word the comment?