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: raise exception when docker version is below 20.0.0 #2895

Merged
merged 2 commits into from
Jul 9, 2021
Merged

feat: raise exception when docker version is below 20.0.0 #2895

merged 2 commits into from
Jul 9, 2021

Conversation

alaeddine-13
Copy link
Contributor

No description provided.

@alaeddine-13 alaeddine-13 requested a review from a team as a code owner July 8, 2021 13:22
@jina-bot jina-bot added size/S area/core This issue/PR affects the core codebase area/helper This issue/PR affects the helper functionality area/network This issue/PR affects network functionality component/peapod labels Jul 8, 2021
@github-actions
Copy link

github-actions bot commented Jul 8, 2021 •
edited

Latency summary

Current PR yields:

  • 😶 index QPS at 1407, delta to last 1 avg.: +3%
  • 😶 query QPS at 25, delta to last 1 avg.: +0%
  • 😶 import jina within 0.2326s, delta to last 1 avg.: +0%

Breakdown

Version Index QPS Query QPS Import Time (s)
current 1407 25 0.2326
2.0.4 1355 24 0.2332

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

@codecov
Copy link

codecov bot commented Jul 8, 2021 •
edited

Codecov Report

Merging #2895 (5d943e8) into master (0094bbf) will increase coverage by 5.83%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2895      +/-   ##
==========================================
+ Coverage   82.21%   88.05%   +5.83%     
==========================================
  Files         106      138      +32     
  Lines        7064     9520    +2456     
==========================================
+ Hits         5808     8383    +2575     
+ Misses       1256     1137     -119
Flag Coverage Δ
daemon 43.41% <ø> (?)
jina 88.00% <ø> (?)

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

Impacted Files Coverage Δ
jina/__init__.py 71.64% <ø> (ø)
jina/checker.py 96.55% <ø> (ø)
jina/clients/__init__.py 100.00% <ø> (ø)
jina/clients/base/__init__.py 90.90% <ø> (ø)
jina/clients/base/grpc.py 63.82% <ø> (ø)
jina/clients/base/http.py 93.18% <ø> (ø)
jina/clients/base/websocket.py 86.36% <ø> (ø)
jina/clients/grpc.py 100.00% <ø> (ø)
jina/clients/helper.py 100.00% <ø> (ø)
jina/clients/http.py 100.00% <ø> (ø)
... and 215 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 b324d51...5d943e8. Read the comment docs.

JoanFM
JoanFM requested changes Jul 8, 2021
View reviewed changes
jina/peapods/runtimes/container/__init__.py Outdated
@@ -81,6 +81,16 @@ def _docker_run(self, replay: bool = False):

client = docker.from_env()

docker_version = client.version().get("Version")
Copy link
Member

Choose a reason for hiding this comment

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

please use single quotes

@JoanFM
Copy link
Member

JoanFM commented Jul 8, 2021

Why is this feature required?

@JoanFM
Copy link
Member

JoanFM commented Jul 8, 2021

Why is this then not shown in the extra-requirements.txt?

@cristianmtr
Copy link
Contributor

Why is this then not shown in the extra-requirements.txt?

It's docker itself, not the Python docker lib

@alaeddine-13
Copy link
Contributor Author

Why is this feature required?

When I was trying to run an executor with jinahub, I receive

docker.errors.APIError: 400 Client Error for http+docker://localhost/v1.40/containers/create?name=pod0ContainerRuntime: Bad Request ("invalid IP address in add-host: "host-gateway"")

Upgrading docker's version from 19 to 20 solves the issue

@JoanFM
Copy link
Member

JoanFM commented Jul 8, 2021

Why is this feature required?

When I was trying to run an executor with jinahub, I receive

docker.errors.APIError: 400 Client Error for http+docker://localhost/v1.40/containers/create?name=pod0ContainerRuntime: Bad Request ("invalid IP address in add-host: "host-gateway"")

Upgrading docker's version from 19 to 20 solves the issue

I think we should understand what's wrong and what exact version is solving this. I think the reason that bumping version solved this is not so strong.

@jina-bot jina-bot added the area/testing This issue/PR affects testing label Jul 8, 2021
@alaeddine-13
Copy link
Contributor Author

Why is this feature required?

When I was trying to run an executor with jinahub, I receive

docker.errors.APIError: 400 Client Error for http+docker://localhost/v1.40/containers/create?name=pod0ContainerRuntime: Bad Request ("invalid IP address in add-host: "host-gateway"")

Upgrading docker's version from 19 to 20 solves the issue

I think we should understand what's wrong and what exact version is solving this. I think the reason that bumping version solved this is not so strong.

I found this issue where they discuss the problem
docker/cli#2664
According to it, the 20.0x versions of the docker daemon support host.docker.internal:host-gateway

@JoanFM
Copy link
Member

JoanFM commented Jul 8, 2021

Why is this feature required?

When I was trying to run an executor with jinahub, I receive

docker.errors.APIError: 400 Client Error for http+docker://localhost/v1.40/containers/create?name=pod0ContainerRuntime: Bad Request ("invalid IP address in add-host: "host-gateway"")

Upgrading docker's version from 19 to 20 solves the issue

I think we should understand what's wrong and what exact version is solving this. I think the reason that bumping version solved this is not so strong.

I found this issue where they discuss the problem
docker/cli#2664
According to it, the 20.0x versions of the docker daemon support host.docker.internal:host-gateway

Good finding then! Let's do this!

@JoanFM
Copy link
Member

JoanFM commented Jul 8, 2021

Why is this feature required?

When I was trying to run an executor with jinahub, I receive

docker.errors.APIError: 400 Client Error for http+docker://localhost/v1.40/containers/create?name=pod0ContainerRuntime: Bad Request ("invalid IP address in add-host: "host-gateway"")

Upgrading docker's version from 19 to 20 solves the issue

I think we should understand what's wrong and what exact version is solving this. I think the reason that bumping version solved this is not so strong.

I found this issue where they discuss the problem
docker/cli#2664
According to it, the 20.0x versions of the docker daemon support host.docker.internal:host-gateway

May u reference this issue in the code or even in the raised exception?

@alaeddine-13 alaeddine-13 force-pushed the feat-docker-version-check branch 2 times, most recently from e8d8648 to 02c0f12 Compare July 8, 2021 23:02
@JoanFM JoanFM merged commit 7226614 into master Jul 9, 2021
@JoanFM JoanFM deleted the feat-docker-version-check branch July 9, 2021 08:41
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

@Kelton8Z Kelton8Z Awaiting requested review from Kelton8Z

@Gikiman Gikiman Awaiting requested review from Gikiman

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@alaeddine-13 @JoanFM @cristianmtr @jina-bot