src-d / engine-deprecated

[DISCONTINUED] Go to https://github.com/src-d/sourced-ce/
https://docs.sourced.tech/engine
Apache License 2.0
217 stars 26 forks source link

Handle missing docker desktop or docker toolbox installation. #417

Closed carlosms closed 5 years ago

carlosms commented 5 years ago

In #415 instead of failing with a panic, srcd should have been able to detect the required docker desktop installation, and fail gracefully.

Scenarios I can think of:

se7entyse7en commented 5 years ago

Docker version is lower than we require

Do we want to asses the minimum version we require and check for that hardcoded value or do we want to also add some kind of testing may be by forcing a different docker api version?

se7entyse7en commented 5 years ago

I actually tried replacing all occurrences in docker.go of client.NewClientWithOpts(client.WithVersion("1.18")) just to give it a try and the integration tests passed. 1.18 is the oldest one listed here.

smacker commented 5 years ago

moby lib already checks API version and returns an error if docker server is too old. I saw it with my docker when updated the lib to master. (v1.40) I'm not sure what client.WithVersion does and did you test it correctly or not but consistency mode was introduced after 1.18 for sure and we rely on it.

se7entyse7en commented 5 years ago

moby lib already checks API version and returns an error if docker server is too old.

Great! didn't know that.

but consistency mode was introduced after 1.18 for sure and we rely on it.

I was thinking the same thing, and I guess that it becomes a noop, but I'm investigating it further. 👍

se7entyse7en commented 5 years ago

@smacker I've just tried to change with client.NewClientWithOpts(client.WithVersion("1.11")) and all the tests fail with:

Error response from daemon: client version 1.11 is too old. Minimum supported API version is 1.12, please upgrade your client to a newer version.

So this means that client.WithVerson actually works.

This also mean that all the newer features simply becomes noop. I manually tried a sample query (Top languages by repository count) with api version 1.12 and 1.38. It works with both version, but with 1.12 it took ~9s and with 1.38 it took ~1.5s.

By running docker version on my local machine the output is:

Screenshot 2019-04-09 at 10 25 59

So it seems that from the minimum version everything works, but it may not work as good as expected. This is aligned to what is stated in the documentation:

A new version of the API is released when new features are added. The Docker API is backward-compatible, so you do not need to update code that uses the API unless you need to take advantage of new features.

Hence I see two options:

  1. we just require that everything simply works, so we could just eventually wrap the docker error message,
  2. we choose a minimum version that we know it makes Engines works as we expect. In our case it could be 1.28 which is the first version with support to mount consistency.
se7entyse7en commented 5 years ago

As discussed here, we assume that the user is running the latest version of Docker and just let the SDK check for API incompatibility. We check then for: