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

Allow http and grpc server bind to specific address #4136

Merged
merged 14 commits into from
Apr 4, 2022
Merged

Allow http and grpc server bind to specific address #4136

merged 14 commits into from
Apr 4, 2022

Conversation

krishung5
Copy link
Contributor

No description provided.

GuanLuo
GuanLuo requested changes Mar 30, 2022
View reviewed changes
Copy link
Contributor

@GuanLuo GuanLuo left a comment

Choose a reason for hiding this comment

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

There is a CheckPortCollision() function in main.cc which you will need to update as well. i.e. different endpoint can bind to the same port as long as they are listening to different address, so the collision should check both the address and port

src/grpc_server.cc Outdated Show resolved Hide resolved
src/http_server.h Outdated Show resolved Hide resolved
@krishung5 krishung5 requested a review from GuanLuo March 31, 2022 16:14
GuanLuo
GuanLuo requested changes Mar 31, 2022
View reviewed changes
src/main.cc Show resolved Hide resolved
@krishung5 krishung5 requested a review from GuanLuo March 31, 2022 17:42
GuanLuo
GuanLuo requested changes Mar 31, 2022
View reviewed changes
Copy link
Contributor

@GuanLuo GuanLuo left a comment

Choose a reason for hiding this comment

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

Also need to add tests for this feature.

src/main.cc Outdated
if (std::get<1>(*curr_it) == std::get<1>(*comparing_it)) {
if (std::get<1>(*curr_it) != std::get<1>(*comparing_it)) {
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to line 640, the range check can be skipped as well if the addresses are different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Thanks for the catch. Updated.

@krishung5 krishung5 requested a review from GuanLuo March 31, 2022 18:08
GuanLuo
GuanLuo reviewed Mar 31, 2022
View reviewed changes
src/main.cc Outdated
@@ -367,6 +373,8 @@ std::vector<Option> options_
"Allow the server to listen for GRPC requests."},
{OPTION_GRPC_PORT, "grpc-port", Option::ArgInt,
"The port for the server to listen on for GRPC requests."},
{OPTION_GRPC_ADDR, "grpc-addr", Option::ArgStr,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: xxx-address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I also make changes to all the parameters/arguments?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should I also make changes to all the parameters/arguments?

Personally I would make the same changes to the variables to keep things consistent. The original ask is because we don't use abbreviation for other cmdline options and the user is likely to assume the same for the new options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks for clarifying. Updated.

@krishung5 krishung5 force-pushed the krish-http-grpc-binding-address branch from f431480 to 3aec851 Compare March 31, 2022 19:37
CoderHam
CoderHam suggested changes Mar 31, 2022
View reviewed changes
src/http_server.cc Outdated
return TRITONSERVER_ErrorNew(
TRITONSERVER_ERROR_UNAVAILABLE,
(std::string("Port '0.0.0.0:") + std::to_string(port_) +
(std::string("Port '") + http_address_ + ":" + std::to_string(port_) +
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix error message here. The port is just port_. A socket is a combination of port and IP address.

src/http_server.h Outdated Show resolved Hide resolved
src/sagemaker_server.h Show resolved Hide resolved
src/vertex_ai_server.cc Outdated Show resolved Hide resolved
@krishung5 krishung5 force-pushed the krish-http-grpc-binding-address branch 3 times, most recently from 2ed295f to efe746e Compare April 1, 2022 15:39
CoderHam
CoderHam previously approved these changes Apr 1, 2022
View reviewed changes
GuanLuo
GuanLuo requested changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@GuanLuo GuanLuo left a comment

Choose a reason for hiding this comment

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

Make sure you also have the corresponding change in CI since the test name is changed

qa/L0_socket/test.sh
fi
run_server_nowait
sleep 15
if [ "$SERVER_PID" == "0" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Should send ready / live request to make sure the server is running.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@CoderHam
Copy link
Contributor

CoderHam commented Apr 1, 2022

@krishung5 you will need to update gitlab yaml for the change in naming for the L0_multiple_ports to avoid breaking the CI

@krishung5
Copy link
Contributor Author

@GuanLuo @CoderHam Thank you both for the heads up! Will update the CI as well.

@krishung5 krishung5 force-pushed the krish-http-grpc-binding-address branch from efe746e to d369037 Compare April 1, 2022 23:12
@krishung5 krishung5 linked an issue Apr 1, 2022 that may be closed by this pull request
Allow http server to bind to specific address/interface #3360
Closed
@krishung5 krishung5 merged commit 776ff6f into main Apr 4, 2022
@krishung5 krishung5 deleted the krish-http-grpc-binding-address branch April 4, 2022 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuanLuo GuanLuo GuanLuo approved these changes

@CoderHam CoderHam Awaiting requested review from CoderHam

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow http server to bind to specific address/interface
3 participants
@krishung5 @CoderHam @GuanLuo