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

Increase NGINX body-size #826

Merged
merged 1 commit into from
Jul 31, 2023
Merged

Increase NGINX body-size #826

merged 1 commit into from
Jul 31, 2023

Conversation

Tansito
Copy link
Member

@Tansito Tansito commented Jul 31, 2023

Summary

Fix #818
Co-authored-by: @psschwei

Confirmed with the infrastructure team and added the required label specified in the issue: proxy-body-size.

@Tansito Tansito requested review from psschwei and IceKhan13 July 31, 2023 10:42
@Tansito Tansito requested a review from akihikokuroda July 31, 2023 10:50
Copy link
Member

@IceKhan13 IceKhan13 left a comment

Choose a reason for hiding this comment

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

🚀

@Tansito Tansito merged commit a5166f1 into main Jul 31, 2023
@Tansito Tansito deleted the increase-body-size branch July 31, 2023 12:56
@akihikokuroda
Copy link
Collaborator

I got a little confused with the changes in the value.yaml file change for the local install. There are 2 ingress instances configurations in the value file. One is installed with Ray operator and the other is installed as a dependency of the quantum-serferless chart. The changes make the change in the ingress installed with Ray operator that is not enable for the local install. The other ingress is enabled for the local install.

@Tansito
Copy link
Member Author

Tansito commented Jul 31, 2023

Mmm... Which instance for Ray operator do you mean, @akihikokuroda ?

Right now I see two different instances:

  • Local NGINX for local development, managed by nginxIngressControllerEnable
  • NGINX configuration for 3rd party cloud platforms

I admit that there we can have a bit of confuse because depending on the provider we are going to need to change that configuration. I'm totally open to improve that (I think @psschwei mentioned before some ways to fix it).
Sorry, I don't know if I'm following you here, @akihikokuroda 🙏

@akihikokuroda
Copy link
Collaborator

I might be still confused. The ingress would be created by this but it is skipped for the local install by this

@psschwei
Copy link
Collaborator

The other ingress is enabled for the local install.

The other ingress being the quantum-serverless dependency, right?

FWIW, I've never used ingress with a local install, always just done port-forwarding of the gateway (and the notebook, if I'm running that via k8s).

@akihikokuroda
Copy link
Collaborator

@psschwei Yes, it is.

@psschwei
Copy link
Collaborator

Ahh, I missed your last comment (need to remember to refresh the page if I leave it open too long before commenting 😄 )

@Tansito
Copy link
Member Author

Tansito commented Aug 1, 2023

I might be still confused. The ingress would be created by this but it is skipped for the local install by this

I see your point and I was not aware about that if. So you are right, @akihikokuroda . We should add in the hosts probably a configuration for that, then. Another discussion that we can have is what @psschwei commented about using NGINX vs port-forwarding in local. In fact, from what I was reading about the notes that @jenglick collected from the users, for local testing most of the people were using the docker-compose not the cluster so... I was reviewing our current UX because I think we can improve it a little bit and even remove the NGINX for local development. I will open a discussion for this, soon.

cc to @IceKhan13 too here 👀

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.

Increase request payload size
4 participants