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.

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

[Feature] Allow RayCluster Helm chart to specify different images for different worker groups #1352

Merged
merged 2 commits into from
Aug 24, 2023

Conversation

Darren221
Copy link
Contributor

@Darren221 Darren221 commented Aug 19, 2023

Why are these changes needed?

Prior to this pull request, the RayCluster Helm chart had uniform default images for various worker groups. This PR introduces a new feature that empowers users to designate distinct images for both worker groups and the head Pod. To ensure backward compatibility, the PR verifies if the "image" section is defined for each Pod. If an image isn't specified in values.yaml, the Pod's image defaults to the option provided at the outset of values.yaml.

Related issue number

Closes #1291

Checks

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

Test 1:

(Link to values.yaml for test 1)

helm install kuberay-operator kuberay/kuberay-operator --version 0.6.0

# Create a RayCluster with 1 head, 1 worker, and 2 worker groups (smallgroup 1 & 2)
# In values.yaml, specify the images for head, worker, and different worker groups
helm install ray-cluster kuberay/helm-chart/ray-cluster --version 0.6.0 -f kuberay/helm-chart/ray-cluster/values.yaml

# Describe the Pod to check its image
# [Expected result]: each Pod has the same image as what was specified (shown in the attached screenshots)
kubectl describe pod $PodName
head_Pod smallgroup1 smallgroup2 worker

Test 2:

(Link to values.yaml for test 2)

helm install kuberay-operator kuberay/kuberay-operator --version 0.6.0

# Create a RayCluster with 1 head, 1 worker, and 2 worker groups (smallgroup 1 & 2)
# In values.yaml, specify the images for worker, and different worker groups. 
# However, in Test 2, the image for head wasn't specified, and the image repository was missed when specifying the image for the worker Pod (to test backward compatibility and error handling).
helm install ray-cluster kuberay/helm-chart/ray-cluster --version 0.6.0 -f kuberay/helm-chart/ray-cluster/values.yaml

# Describe the Pod to check its image
# [Expected result]: 
# The image for the head was the same as the default option since it was not specified in values.yaml. 
# Additionally, an error (InvalidImageName) appeared in the worker Pod's description due to a missed image repository specification. This was done to alert the user. 
# Apart from these points, the images for other Pods matched those of Test 1, in line with what was defined in values.yaml.
kubectl describe pod $PodName
head_Pod(default) InvalidImageName

… different worker for different worker groups
@Darren221 Darren221 closed this Aug 21, 2023
@Darren221 Darren221 changed the title [Feature] Allow RayCluster Helm chart to specify different images for… [Feature] Allow RayCluster Helm chart to specify different images for different worker groups Aug 21, 2023
@Darren221 Darren221 reopened this Aug 21, 2023
@kevin85421 kevin85421 self-requested a review August 21, 2023 16:50
@@ -48,8 +48,13 @@ spec:
containers:
- volumeMounts: {{- toYaml .Values.head.volumeMounts | nindent 12 }}
name: ray-head
{{- if .Values.head.image }}
image: {{ .Values.head.image.repository }}:{{ .Values.head.image.tag }}
imagePullPolicy: {{ .Values.head.image.pullPolicy }}
Copy link
Member

Choose a reason for hiding this comment

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

What will happen if pullPolicy is not defined?

Copy link
Member

Choose a reason for hiding this comment

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

ImagePullPolicy is an optional field for Pod, and the default value is here.

Copy link
Contributor Author

@Darren221 Darren221 Aug 22, 2023

Choose a reason for hiding this comment

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

To find out the difference between Pod and this PR regarding the imagePullPolicy, two test cases are shown below with different values for the tag of the image.
In this PR, both the repository and tag must be provided when specifying the image section.

Regarding the imagePullPolicy:
if it's left unspecified and the tag isn't set to "latest", the default becomes "IfNotPresent". (As shown in the figure below)

values.yaml (tag: not latest)
Screenshot 2023-08-22 at 15 19 47
Screenshot 2023-08-22 at 15 20 56

However, if the tag is "latest" and imagePullPolicy is not set, the default is "Always". (As shown in the figure below)

values.yaml (tag: latest)
Screenshot 2023-08-22 at 15 27 24
Screenshot 2023-08-22 at 15 24 47

@nicklausbrown
Copy link

nicklausbrown commented Aug 23, 2023

When this is merged, will it automatically build a newly available helm chart, or will this be included in a future release (e.g. 0.7.0)? @Darren221, thanks for the work here 😄 and @kevin85421 appreciate your work maintaining the project 😄

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.

LGTM

@kevin85421
Copy link
Member

When this is merged, will it automatically build a newly available helm chart, or will this be included in a future release (e.g. 0.7.0)? @Darren221, thanks for the work here 😄 and @kevin85421 appreciate your work maintaining the project 😄

Thank @nicklausbrown for reaching out! Currently, CI will not publish the nightly chart automatically, but the first release candidate for Ray Summit (mid Sept) will be out soon.

@kevin85421 kevin85421 merged commit de8bc26 into ray-project:master Aug 24, 2023
38 of 39 checks passed
blublinsky pushed a commit to blublinsky/kuberay that referenced this pull request Aug 25, 2023
… different worker groups (ray-project#1352)

Allow RayCluster Helm chart to specify different images for different worker groups
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
… different worker groups (ray-project#1352)

Allow RayCluster Helm chart to specify different images for different worker groups
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.

[Feature] Allow RayCluster Helm chart to specify different images for different worker groups
3 participants