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: allow multiple port and protocols for gateway #5378

Merged
merged 81 commits into from Nov 18, 2022
Merged

feat: allow multiple port and protocols for gateway #5378

merged 81 commits into from Nov 18, 2022

Conversation

alaeddine-13
Copy link
Contributor

@alaeddine-13 alaeddine-13 commented Nov 11, 2022 •
edited

This PR adds support for multiple ports and protocols for the gateway runtime.

  • Gateway parser can receive list or single value
  • Flow __init__ and config_gateway can receive multiple or single value. Flow.port and Flow.protocol return single or multiple values
  • GatewayRuntime expects list of values
  • BaseGateway can receive runtime args
  • (optional) move injected dependencies to runtime_args, TBD in inject streamer and other dependencies to the gateway class at construction time using runtime_args #5396
  • implemented Gateway classes uses the port and protocol from runtime args
  • support in JAML
  • k8s gateway service
  • docker compose gateway service
  • health checks
  • NetworkChecker
  • docker and k8s checks
  • Flow pod checks

@github-actions github-actions bot added size/M area/core This issue/PR affects the core codebase area/helper This issue/PR affects the helper functionality area/testing This issue/PR affects testing component/jaml area/docs This issue/PR affects the docs component/client labels Nov 11, 2022
@codecov
Copy link

codecov bot commented Nov 11, 2022 •
edited

Codecov Report

Merging #5378 (5d6dc05) into master (63cde2d) will decrease coverage by 0.06%.
The diff coverage is 93.13%.

@@            Coverage Diff             @@
##           master    #5378      +/-   ##
==========================================
- Coverage   86.99%   86.93%   -0.07%     
==========================================
  Files         101      101              
  Lines        6606     6667      +61     
==========================================
+ Hits         5747     5796      +49     
- Misses        859      871      +12
Flag Coverage Δ
jina 86.93% <93.13%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
jina/serve/runtimes/base.py 74.19% <ø> (-0.81%) ⬇️
jina/serve/gateway.py 92.45% <83.33%> (-1.30%) ⬇️
jina/orchestrate/flow/base.py 90.47% <85.71%> (-0.30%) ⬇️
jina/enums.py 82.17% <100.00%> (-4.14%) ⬇️
...a/orchestrate/deployments/config/docker_compose.py 99.01% <100.00%> (ø)
...deployments/config/k8slib/kubernetes_deployment.py 63.07% <100.00%> (+5.18%) ⬆️
jina/orchestrate/pods/container.py 87.06% <100.00%> (+0.26%) ⬆️
jina/orchestrate/pods/helper.py 90.38% <100.00%> (ø)
jina/parsers/client.py 100.00% <100.00%> (ø)
jina/parsers/helper.py 52.63% <100.00%> (+5.18%) ⬆️
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

JoanFM
JoanFM requested changes Nov 11, 2022
View reviewed changes
jina/orchestrate/flow/base.py Outdated Show resolved Hide resolved
jina/parsers/orchestrate/pod.py
'--port', '--port-in', type=str, default=str(helper.random_port()), help=port_description
)
else:
gp.add_argument(
Copy link
Member

Choose a reason for hiding this comment

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

can u show me a CLI example of how does this work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jina gateway --port 12345 12345
or
jina gateway --port 12345

Copy link
Member

Choose a reason for hiding this comment

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

why didn't we do like this in the past for other cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure, probably we were just fine with the comma separated list of values format

jina/parsers/orchestrate/runtimes/remote.py

parser.add_argument(
'--protocol',
nargs='+',
Copy link
Member

Choose a reason for hiding this comment

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

we need to have a plural alias

Copy link
Member

Choose a reason for hiding this comment

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

What happens if I pass a single one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be read as a list of protocls with 1 element. You can check the behavior here
tests/unit/parsers/peapods/runtimes/test_port_protocol_parser.py

jina/parsers/orchestrate/pod.py
)
else:
gp.add_argument(
'--port',
Copy link
Member

Choose a reason for hiding this comment

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

we need a plural alias for gateway

Copy link
Member

Choose a reason for hiding this comment

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

I believe with an action we can make sure that these are passed as int to the Python objects so that the code is much cleaner afterwards

Copy link
Member

Choose a reason for hiding this comment

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

Add ports plural alias

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@github-actions github-actions bot removed area/docs This issue/PR affects the docs component/client labels Nov 11, 2022
@alaeddine-13 alaeddine-13 reopened this Nov 11, 2022
@github-actions github-actions bot added area/docs This issue/PR affects the docs component/client labels Nov 11, 2022
@github-actions
Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

1 similar comment
@github-actions
Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@alaeddine-13 alaeddine-13 mentioned this pull request Nov 17, 2022
Merged
@github-actions
Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

JoanFM
JoanFM previously approved these changes Nov 17, 2022
View reviewed changes
@JoanFM JoanFM linked an issue Nov 17, 2022 that may be closed by this pull request
make port argument support multiple ports #5343
Closed
@github-actions
Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@github-actions
Copy link

📝 Docs are deployed on https://feat-gateway-ports--jina-docs.netlify.app 🎉

@JoanFM JoanFM merged commit de5942f into master Nov 18, 2022
@JoanFM JoanFM deleted the feat-gateway-ports branch November 18, 2022 10:04
@alexcg1 alexcg1 mentioned this pull request Nov 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@girishc13 girishc13 girishc13 left review comments

@samsja samsja samsja left review comments

@JoanFM JoanFM JoanFM approved these changes

Assignees
No one assigned
Labels
area/cli This issue/PR affects the command line interface area/core This issue/PR affects the core codebase area/docs This issue/PR affects the docs area/helper This issue/PR affects the helper functionality area/testing This issue/PR affects testing component/jaml component/resource size/L size/M size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Customize the output when using custom gateway make port argument support multiple ports
5 participants
@alaeddine-13 @girishc13 @JoanFM @samsja @jina-bot