8000 refactor: change bind to all inbound by slettner · Pull Request #3069 · jina-ai/serve · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor: change bind to all inbound #3069

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
Merged
merged 2 commits into from
Aug 3, 2021
Merged

refactor: change bind to all inbound #3069

merged 2 commits into from
Aug 3, 2021

Conversation

slettner
Copy link
Contributor
@slettner slettner commented Aug 2, 2021

This PR introduces the following change:

For all runtimes, the gateway will bind to all inbound connections on the localhost, i.e. 0.0.0.0 instead of the parameter args.host.

This change is motivated by a use-case from kubernetes where the gateway host deployment is hidden behind a kubernetes service with corresponding dns (this is a typical kubernetes pattern).
Clients can reach the gateway via this dns from the outside but the host cannot bind to this address as it is the IP of the service not the deployment where the gateway runs.

@slettner slettner requested a review from maximilianwerk August 2, 2021 11:49
@slettner slettner self-assigned this Aug 2, 2021
@slettner slettner requested a review from a team as a code owner August 2, 2021 11:49
@slettner slettner requested a review from nan-wang August 2, 2021 11:49
@github-actions github-actions bot added size/XS area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality component/peapod labels Aug 2, 2021
@codecov
Copy link
codecov bot commented Aug 2, 2021 •
edited
Loading

Codecov Report

Merging #3069 (aa32640) into master (27d93c2) will increase coverage by 3.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3069      +/-   ##
==========================================
+ Coverage   86.21%   89.35%   +3.13%     
==========================================
  Files         142      142              
  Lines        9503     9506       +3     
==========================================
+ Hits         8193     8494     +301     
+ Misses       1310     1012     -298
Flag Coverage Δ
daemon 43.77% <100.00%> (+0.01%) ⬆️
jina 89.34% <100.00%> (+3.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jina/peapods/runtimes/gateway/grpc/__init__.py 100.00% <100.00%> (ø)
jina/peapods/runtimes/gateway/http/__init__.py 96.77% <100.00%> (+0.10%) ⬆️
...ina/peapods/runtimes/gateway/websocket/__init__.py 96.77% <100.00%> (+0.10%) ⬆️
jina/helper.py 79.92% <0.00%> (+0.37%) ⬆️
jina/jaml/parsers/__init__.py 100.00% <0.00%> (+7.31%) ⬆️
jina/optimizers/discovery.py 80.26% <0.00%> (+80.26%) ⬆️
jina/optimizers/__init__.py 97.02% <0.00%> (+97.02%) ⬆️
jina/optimizers/flow_runner.py 97.67% <0.00%> (+97.67%) ⬆️
jina/optimizers/parameters.py 98.38% <0.00%> (+98.38%) ⬆️
jina/parsers/optimizer.py 100.00% <0.00%> (+100.00%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27d93c2...aa32640. Read the comment docs.

@github-actions
Copy link
github-actions bot commented Aug 2, 2021 •
ed 8000 ited
Loading

Latency summary

Current PR yields:

  • 🐢🐢 index QPS at 1438, delta to last 2 avg.: -15%
  • 🐢🐢 query QPS at 39, delta to last 2 avg.: -13%
  • 🐢🐢 dam extend QPS at 70352, delta to last 2 avg.: -12%
  • 🐢🐢 avg flow time within 1.2929 seconds, delta to last 2 avg.: +8%
  • 🐎🐎🐎🐎 import jina within 0.3644 seconds, delta to last 2 avg.: +13%

Breakdown

Version Index QPS Query QPS DAM Extend QPS Avg Flow Time (s) Import Time (s)
current 1438 39 70352 1.2929 0.3644
2.0.15 1505 40 71601 1.2296 0.346
2.0.14 1899 49 90122 1.1606 0.295

Backed by latency-tracking. Further commits will update this comment.

@github-actions github-actions bot added size/S and removed size/XS labels Aug 2, 2021
@maximilianwerk
Copy link
Member

@JoanFM @deepankarm any objections?

@JoanFM
Copy link
Contributor
JoanFM commented Aug 2, 2021

it seems good. It surprises me how it used to work before? Binding is always local to one self right?

@JoanFM
Copy link
Contributor
JoanFM commented Aug 2, 2021

What I do not understand is then why uvicorn exposes the host argument.

In the meanwhile, u can simply set properly the host argument right @slettner .

I would wait for @deepankarm input on this

JoanFM
JoanFM suggested changes Aug 2, 2021
View reviewed changes
Copy link
Contributor
@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

Wait for @deepankarm input

@hanxiao
Copy link
Member
hanxiao commented Aug 3, 2021 •
edited
Loading

It surprises me how it used to work before? Binding is always local to one self right?

Because arg.host by default is __default_host__

Note that this PR is not complete, if in those contexts host is no longer a parameter but a constant, then argparse should not expose them to the users. The parser side needs to be refactored a bit to remove host for those context.

@maximilianwerk
Copy link
Member
maximilianwerk commented Aug 3, 2021 •
edited
Loading

It surprises me how it used to work before? Binding is always local to one self right?

Because arg.host by default is __default_host__

Note that this PR is not complete, if in those contexts host is no longer a parameter but a constant, then argparse should not expose them to the users. The parser side needs to be refactored a bit to remove host for those context.

That's wrong. We still need the host to tell the last Pod how to reach the Gateway again. For this, we need to configure the host of the Gateway. With dynamic routing, you never define host_in and host_out by yourself, but only tell each Pod on which host it runs. The Flow then computes the relevant information for either the routing_table or sets host_in if we have a remote-local situation.

@maximilianwerk
Copy link
Member

To give some more motivation, imagine the following Flow:

Flow.add(name='p1').add(name='p2').add(name='p3', needs=['p1', 'p2'])

You can now easily put them on different machines via setting host of each Pod. Anyhow, if you want to configure the host_in of p3, what would it be - p1.host or p2.host? Same holds true for host_out of p1 - p2.host or p3.host?

@hanxiao
Copy link
Member
hanxiao commented Aug 3, 2021 •
edited
Loading

The parser side needs to be refactored a bit to remove host for those context.

context means every context mentioned in this PR. Not beyond this PR.

Every parser behind these contexts should remove host as argument. Any parser that not related to this PR should stay the same.

@maximilianwerk
Copy link
Member

The parser side needs to be refactored a bit to remove host for those context.

context means every context mentioned in this PR. Not beyond this PR.

Every parser behind these contexts should remove host as argument. Any parser that not related to this PR should stay the same.

The host argument is currently used in four places:

  • Daemon => still needed
  • Pea => still needed
  • Gateway => still needed as described above
  • ClientCLI => still needed or at least this PR does not change anything with the ClientCLI

The Gateway also uses the mixin_http_gateway_parser, which actually does not has the host argument and thus, we don't need to change anything. Perhaps this should have had the host argument beforehand, but it does not. GRPC and websocket do not have their own mixins at all.

Am I getting you right, if you would prefer to split the configuration into further mixins with these arguments specified more detailed and as low level as possible?

@maximilianwerk maximilianwerk merged commit ed3ff21 into master Aug 3, 2021
@maximilianwerk maximilianwerk deleted the refactor-host-address-http branch August 3, 2021 12:41
davidbp pushed a commit that referenced this pull request Aug 4, 2021
@slettner @davidbp
refactor: change bind to all inbound (#3069)
9e278ac 
* refactor: change bind to all inbound

* refactor: use default host
@hanxiao hanxiao mentioned this pull request Aug 9, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoanFM JoanFM JoanFM approved these changes

@maximilianwerk maximilianwerk Awaiting requested review from maximilianwerk

@nan-wang nan-wang Awaiting requested review from nan-wang

Assignees

@slettner slettner

Labels
area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality component/peapod size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@slettner @maximilianwerk @JoanFM @hanxiao
0