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

Add a flag to enable/disable worker init container injection #1069

Merged
merged 14 commits into from
May 10, 2023

Conversation

ByronHsu
Copy link
Contributor

@ByronHsu ByronHsu commented May 6, 2023

Why are these changes needed?

Currently, the init container is always injected into the worker pod with a specific command. However, in our case, ray does not exist in the $PATH so it failed. We want to have a way to disable the injection from kuberay and customize our own injection. This PR adds an env ENABLE_INIT_CONTAINER_INJECTION to switch the init container injection.

Checks

  • I've made sure the tests are passing.
Running Suite: Utils Suite
==========================
Random Seed: 1683590005
Will run 4 of 4 specs

••••
Ran 4 of 4 Specs in 0.001 seconds
SUCCESS! -- 4 Passed | 0 Failed | 0 Pending | 0 Skipped
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

ray-operator/go.mod Outdated Show resolved Hide resolved
@ByronHsu ByronHsu changed the title [WIP] Allow customizing worker init container image Add a flag to enable/disable worker init container injection May 8, 2023
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.

Add this env variable in KubeRay operator Helm chart with the default value "true" and warning comments.

ray-operator/go.mod Outdated Show resolved Hide resolved
ray-operator/controllers/ray/common/pod.go Show resolved Hide resolved
@kevin85421
Copy link
Member

@ByronHsu Here is a conflict. Would you mind rebasing with the master branch?

@kevin85421
Copy link
Member

Would you mind fixing the lint error and adding this env variable in KubeRay operator Helm chart with the default value "true" and warning comments? Thanks!

@ByronHsu
Copy link
Contributor Author

e2e test done

  1. Local build the image and push
  2. Add the env and update the image to the chart
- name: ENABLE_INIT_CONTAINER_INJECTION
  value: "false"
  1. Init Containers doesn't have ray health-check anymore, only containing my custom init container
Init Containers:
  init-myservice:
     <masked>

Signed-off-by: byhsu <[email protected]>
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. Thank you for this contribution!

I also tested it manually.

# path: helm-chart/kuberay-operator
# Step 1: Update values.yaml
- name: ENABLE_INIT_CONTAINER_INJECTION
  value: "false"

# Step 2: Install the KubeRay operator with the new image
helm install kuberay-operator . --set image.repository=controller,image.tag=latest

# Step 3: Install a RayCluster with a init container "busybox"
helm install raycluster kuberay/ray-cluster --version 0.5.0

# Step 4: The worker Pod will only have 1 init container.
# path: helm-chart/kuberay-operator
# Step 1: Update values.yaml
- name: ENABLE_INIT_CONTAINER_INJECTION
  value: "true"

# Step 2: Install the KubeRay operator with the new image
helm install kuberay-operator . --set image.repository=controller,image.tag=latest

# Step 3: Install a RayCluster with a init container "busybox"
helm install raycluster kuberay/ray-cluster --version 0.5.0

# Step 4: The worker Pod will have 2 init containers.

@kevin85421 kevin85421 merged commit b92d95a into ray-project:master May 10, 2023
@kevin85421 kevin85421 mentioned this pull request Jul 7, 2023
1 task
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
…ject#1069)

Add a flag to enable/disable worker init container injection
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.

2 participants