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.

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

Upgrade docker-py to its latest version (docker==3.1.1) #3243

Merged
merged 11 commits into from Mar 15, 2018
Merged

Conversation

@humitos
Copy link
Member

@humitos humitos commented Nov 10, 2017

To avoid this issue immediately after setting up the dev environment:

[10/Nov/2017 12:15:43] readthedocs.doc_builder.environments:316[12710]: ERROR (Build) [fdfdd:latest] mem_limit has been moved to host_config in API version 1.19

I tried updating the docker-py version used and the update the code when creating the container to set the proper mem_limit under host_config.

Since I don't know what's the version of docker that we are running in production, maybe this PR is useless. Just in case, I'm pasting here what's my local version where it's working with this new upgrade.

(rtfd) [humitos@julia:readthedocs.org|upgrade-dockerpy *$%]$ docker version
Client:
 Version:      17.10.0-ce
 API version:  1.33
 Go version:   go1.9.1
 Git commit:   f4ffd2511c
 Built:        Wed Oct 18 23:08:56 2017
 OS/Arch:      linux/amd64

Server:
 Version:      17.10.0-ce
 API version:  1.33 (minimum version 1.12)
 Go version:   go1.9.1
 Git commit:   f4ffd2511c
 Built:        Wed Oct 18 23:09:11 2017
 OS/Arch:      linux/amd64
 Experimental: false
(rtfd) [humitos@julia:readthedocs.org|upgrade-dockerpy *$%]$ docker info
Containers: 13
 Running: 0
 Paused: 0
 Stopped: 13
Images: 28
Server Version: 17.10.0-ce
Storage Driver: overlay2
 Backing Filesystem: extfs
 Supports d_type: true
 Native Overlay Diff: false
Logging Driver: json-file
Cgroup Driver: cgroupfs
Plugins:
 Volume: local
 Network: bridge host macvlan null overlay
 Log: awslogs fluentd gcplogs gelf journald json-file logentries splunk syslog
Swarm: inactive
Runtimes: runc
Default Runtime: runc
Init Binary: docker-init
containerd version: 06b9cb35161009dcb7123345749fef02f7cea8e0
runc version: 0351df1c5a66838d0c392b4ac4cf9450de844e2d
init version: 949e6fa
Security Options:
 seccomp
  Profile: default
Kernel Version: 4.13.11-1-ARCH
Operating System: Arch Linux
OSType: linux
Architecture: x86_64
CPUs: 4
Total Memory: 7.554GiB
Name: julia
ID: MEOO:ZCSX:Q7E6:2LVZ:5ECY:VYPD:AMZU:ZW6R:JSMH:Z3NF:RKNQ:IFTI
Docker Root Dir: /var/lib/docker
Debug Mode (client): false
Debug Mode (server): false
Registry: https://index.docker.io/v1/
Experimental: false
Insecure Registries:
 127.0.0.0/8
Live Restore Enabled: false

(rtfd) [humitos@julia:readthedocs.org|upgrade-dockerpy *$%]$ 
@humitos
Copy link
Member Author

@humitos humitos commented Nov 10, 2017

Otherwise, to avoid that issue, I have to force DOCKER_VERSION = '1.17' in the dev.py settings file

@humitos
Copy link
Member Author

@humitos humitos commented Nov 10, 2017

docker-py was replaced by docker and its latest version is 2.6.1 but it needs more code changes. So, we need to decide if worth it.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Nov 11, 2017

The current server version is 1.7.1, server API version support is 1.19. No reason we can't upgrade, but this hasn't been a priority yet. I think this upgrade should be fine. We should also update the default version in our settings.

Copy link
Contributor

@agjohnson agjohnson left a comment

We should hardcode the minimum client version, or I guess just 1.19 for now, in readthedocs.settings.DOCKER_VERSION

@humitos
Copy link
Member Author

@humitos humitos commented Nov 16, 2017

I hardcoded the 1.19 docker version in the base settings.

Let me know if this is the way to go. Besides, there is another PR #2916 that tries to upgrade all the packages using pur command.

@humitos
Copy link
Member Author

@humitos humitos commented Nov 16, 2017

On the other hand, docker-py was replaced just by docker. So, that could be another thing that we can take a look (it could be later in another PR)

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Nov 27, 2017

We probably want to coordinate the upgrade of Docker with this PR. I'll create an issue in our provisioning. Blocking here for now.

@humitos
Copy link
Member Author

@humitos humitos commented Dec 11, 2017

@agjohnson did you already upgrade the version in the server?

Do you want me to replace docker-py by docker?

Otherwise, what is missing here to merge the PR?

@humitos humitos force-pushed the upgrade-dockerpy branch from dffc276 to caff893 Dec 12, 2017
@humitos
Copy link
Member Author

@humitos humitos commented Dec 12, 2017

@agjohnson I upgraded to the latest docker and latest (possible) requests -- there is a bug on this lib that has a hotfix but it doesn't work very well and there are many users that can't upgrade to the latest requests because of this.

The minimum supported API version is now 1.21 (Engine version 1.9.0+)

References:

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Dec 16, 2017

Cool. This probably looks good then. I'll plan on deploying the new docker next week and then going for this with the next deploys as well.

@@ -52,7 +55,7 @@ stripe==1.20.2
django-copyright==1.0.0
django-formtools==1.0
django-dynamic-fixture==1.8.5
docker-py==1.3.1
docker==2.6.1
Copy link
Member Author

@humitos humitos Dec 27, 2017

2.7.0 is out

Copy link
Member Author

@humitos humitos Dec 27, 2017

from its changelog it seems to be safe to use it https://docker-py.readthedocs.io/en/stable/change-log.html#

besides, I just tested it locally and it just works

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Mar 8, 2018

Whoops, looks like tests are failing here. Let's see if we can get this into next week's deploy, along with the package upgrades.

@humitos
Copy link
Member Author

@humitos humitos commented Mar 8, 2018

I think you made a mistake when you did the merge, since now we are importing

from docker import Client

and it should be

from docker import APIClient

I will take a deep look to the merge and fix it.

@humitos
Copy link
Member Author

@humitos humitos commented Mar 8, 2018

Docker 3.1.1 is released, so I will try to upgrade to that one after fixing the conflicts.

https://pypi.python.org/pypi/docker/

@humitos
Copy link
Member Author

@humitos humitos commented Mar 8, 2018

To QA this manually, you need to re-create the whole venv. Otherwise, you will find weird issues with the old docker-py package.

@humitos humitos changed the title Upgrade docker-py to its latest version (1.10.6) Upgrade docker-py to its latest version (docker==3.1.1) Mar 9, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Mar 9, 2018

@agjohnson I also removed the DOCKER_VERSION to use the default (auto). I did some QA on .com and .org and I didn't find any issue with this new packages, so I'd say that we can merge this together with #2916 to start testing all of this while developing and deploy next mid-week.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Mar 9, 2018

What is the functionality of auto? We've explicitly set an API version in our settings so that our API schema is guaranteed to be correct. I hesitate to make this auto select if it just grabs the version from the docker api and disregards the version of api schema we're actually using.

@humitos
Copy link
Member Author

@humitos humitos commented Mar 9, 2018

@agjohnson auto will automatically detect the version from the server:

https://docker-py.readthedocs.io/en/stable/client.html#docker.client.from_env

The version of the API to use. Set to auto to automatically detect the server’s version. Default: 1.30

We can use a fixed one here without problem, but I'd suggest to query the docker version in our servers first and use the latest possible there.

In my case, the auto is selecting 1.36 which is the latest that the version of docker that I have installed supports --but it could use up to 1.12 as minimum version:

$ docker version

....

Server:
 Engine:
  Version:	18.02.0-ce
  API version:	1.36 (minimum version 1.12)

Also also, we don't have this version pinned for local development (we use auto as default),

https://github.com/rtfd/readthedocs.org/blob/e923c0cdf7547e63b4d2a9ab2b5e69b527bb482b/readthedocs/doc_builder/constants.py#L37

but I would check the servers and pin it in our provisioning scripts.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Mar 10, 2018

auto might be fine, I'm not sure. We'll see this in development before we see it in production if we always use auto though, so I suppose this should just be fine.

@humitos
Copy link
Member Author

@humitos humitos commented Mar 10, 2018

The default was auto but @stsewd and I were using 1.18 for some time because some incompatibility between docker-py (client) and the docker server installed in the system.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Mar 10, 2018

Same, i have this pinned locally as well.

@agjohnson agjohnson merged commit d30cc12 into master Mar 15, 2018
1 check passed
@agjohnson agjohnson deleted the upgrade-dockerpy branch Mar 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants