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.

Sign up for GitHub

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

Jump to bottom

feat: support ipv6 addresses for serve #3914

Merged
merged 1 commit into from
Oct 10, 2023
Merged

feat: support ipv6 addresses for serve #3914

merged 1 commit into from
Oct 10, 2023

Conversation

sauyon
Copy link
Contributor

@sauyon sauyon commented May 31, 2023 •
edited
Loading

No longer listens on ipv6 catchall by default.

@sauyon sauyon requested a review from a team as a code owner May 31, 2023 16:34
@sauyon sauyon requested review from parano and removed request for a team May 31, 2023 16:34
aarnphm
aarnphm previously approved these changes May 31, 2023
View reviewed changes
aarnphm
aarnphm reviewed Jun 12, 2023
View reviewed changes
src/bentoml/server.py Outdated Show resolved Hide resolved
aarnphm
aarnphm previously approved these changes Jun 15, 2023
View reviewed changes
@aarnphm
Copy link
Contributor

aarnphm commented Jun 15, 2023

@sauyon hmm seems like the monitoring tests is failing on Windows :)

@sauyon
Copy link
Contributor Author

sauyon commented Aug 29, 2023

@aarnphm can you look at this one again?

@sauyon
Copy link
Contributor Author

sauyon commented Aug 30, 2023

@bojiang it seems that AF_INET6 doesn't give us ipv4 for free on Windows; I don't think it's worth taking the effort to fix this for now.

@sauyon
Copy link
Contributor Author

sauyon commented Aug 30, 2023

Actually, just reverted serving by default on ::; this should be fine now!

@aarnphm
Copy link
Contributor

aarnphm commented Aug 31, 2023

@sauyon seems like the windows tests are still failing

@aessiane
Copy link

Hey guys! Having this working would be terrific for our team, do you have an idea of how likely this is to be merged?

@sauyon
Copy link
Contributor Author

sauyon commented Sep 12, 2023

Have been ignoring this because of Windows CI bugs, but will bump this one!

@aarnphm
Copy link
Contributor

aarnphm commented Sep 12, 2023

@sauyon if the windows bug is hard to fix then should we just warn windows users that ipv6 won't work for now?

@sauyon
Copy link
Contributor Author

sauyon commented Sep 12, 2023

Oddly it's not even that ipv6 doesn't work; it seems to work just fine both v4 and v6. Something's wrong with the test.

@aarnphm
Copy link
Contributor

aarnphm commented Sep 12, 2023

Ok lets just merge then?

@aarnphm
Copy link
Contributor

aarnphm commented Sep 12, 2023

One day we will fix our tests for once.

@sauyon
Copy link
Contributor Author

sauyon commented Sep 12, 2023

I'm working on it now; if it's not done by next release we can probably merge this, yeah.

@sauyon
Copy link
Contributor Author

sauyon commented Oct 10, 2023

@aarnphm let's merge this now? dropped test work, clearly..

@aarnphm aarnphm merged commit 23ff8b9 into bentoml:main Oct 10, 2023
38 of 42 checks passed
@aarnphm aarnphm deleted the ipv6 branch October 10, 2023 02:55
@larme larme added this to the 1.1.7 milestone Oct 10, 2023
@aarnphm aarnphm mentioned this pull request Oct 12, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarnphm aarnphm aarnphm approved these changes

@parano parano Awaiting requested review from parano parano is a code owner automatically assigned from bentoml/bentoml-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.1.7
Development

Successfully merging this pull request may close these issues.
4 participants
@sauyon @aarnphm @aessiane @larme