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

Deprecate built in ingress #911

Closed

Conversation

kevin85421
Copy link
Member

@kevin85421 kevin85421 commented Feb 16, 2023

Why are these changes needed?

As we mentioned in ingress.md, we do not recommend using built-in ingress support in a production environment with complex routing requirements.

KubeRay built-in ingress support: KubeRay will help users create a Kubernetes ingress when spec.headGroupSpec.enableIngress: true. Currently, the built-in support only supports simple NGINX setups. Note that users still need to install ingress controller by themselves. We do not recommend using built-in ingress support in a production environment with complex routing requirements.

Related issue number

#905

With the PR, the KubeRay operator will not report the error in #905 (comment). However, the RayCluster chart and YAML are still incompatible with Kubernetes 1.18. Will work on the following if this PR is approved by the community.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(
# Step 1: Create a RayCluster.
kind create cluster --image=kindest/node:v1.18.15

# Step 2: Clone this branch, and build the Docker image.
# Step 3: Load the Docker image into the Kind cluster.
kind load docker-image controller:latest

# Step 4: Install the KubeRay operator chart with the new image
helm install kuberay-operator kuberay/kuberay-operator --version 0.4.0 --set image.repository=controller,image.tag=latest

@Jeffwan
Copy link
Collaborator

Jeffwan commented Feb 17, 2023

This is a breaking change and I don't suggest to deprecate it immediately. As you said, you don't suggest to use with complex rules. However, lots of user use simplified rules. Why not to make it optional but deleting the code? Even if we want to deprecate a feature, it should have an exit plan.

@kevin85421
Copy link
Member Author

This is a breaking change and I don't suggest to deprecate it immediately. As you said, you don't suggest to use with complex rules. However, lots of user use simplified rules. Why not to make it optional but deleting the code? Even if we want to deprecate a feature, it should have an exit plan.

Thank @Jeffwan for the comments!

I agree with you that the deprecation is a bit aggressive. This is a tradeoff between:

  • Pros:

    • Should KubeRay support Kubernetes 1.18? If yes, we need to deprecate the built-in ingress support.
    • Performance improvement (not sure, but the reconciler does not need to reconcile ingress events after it is deprecated.)
  • Cons:

    • The deprecation breaks the backward compatibility. However, the cost is not that expensive because the feature starts to work in release 0.4.0 (#646), and we have a document to deemphasize the built-in ingress support at that moment.

A user wants to deploy KubeRay on Kubernetes 1.18 (#905). If the KubeRay community does not want to support Kubernetes 1.18, it it unnecessary to deprecate the built-in ingress support. On the other hand, we need to deprecate it. Maybe we can discuss this topic in the next community sync.

@Jeffwan
Copy link
Collaborator

Jeffwan commented Feb 21, 2023

@kevin85421 if supporting 1.18 is the case, I still suggest not to delete it. As you know, ingress API was changed as well and there're breaking changes for sure. Another thing to notice is 1.18 was sunset for long time in most public clouds. I suggest we open a new branch and merge the code there, WDYT? Users can build images by themselves.

@kevin85421 kevin85421 mentioned this pull request Jun 23, 2023
1 task
@kevin85421 kevin85421 closed this Aug 24, 2023
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