From a74d546864b64ba03dce1e3407d3b0cd5badee9f Mon Sep 17 00:00:00 2001 From: adw1n Date: Mon, 3 Sep 2018 05:54:12 +0200 Subject: [PATCH 1/2] Fix pulling images with `stream=True` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pulling an image with option `stream=True` like this: ``` client.api.pull('docker.io/user/repo_name', tag='latest', stream=True) ``` without consuming the generator oftentimes results in premature drop of the connection. Docker daemon tries to send progress of pulling the image to the client, but it encounters an error (broken pipe) and therefore cancells the pull action: ``` Thread 1 "dockerd-dev" received signal SIGPIPE, Broken pipe. ERRO[2018-09-03T05:12:35.746497638+02:00] Not continuing with pull after error: context canceled ``` As described in issue #2116, even though client receives response with status code 200, image is not pulled. Closes #2116 Signed-off-by: Przemysław Adamek --- docker/api/image.py | 3 ++- docker/models/images.py | 1 + tests/unit/models_containers_test.py | 3 ++- tests/unit/models_images_test.py | 6 ++++-- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/docker/api/image.py b/docker/api/image.py index a9f801e93b..d3fed5c0cc 100644 --- a/docker/api/image.py +++ b/docker/api/image.py @@ -334,7 +334,8 @@ def pull(self, repository, tag=None, stream=False, auth_config=None, Args: repository (str): The repository to pull tag (str): The tag to pull - stream (bool): Stream the output as a generator + stream (bool): Stream the output as a generator. Make sure to + consume the generator, otherwise pull might get cancelled. auth_config (dict): Override the credentials that :py:meth:`~docker.api.daemon.DaemonApiMixin.login` has set for this request. ``auth_config`` should contain the ``username`` diff --git a/docker/models/images.py b/docker/models/images.py index 4578c0bd89..f8b842a943 100644 --- a/docker/models/images.py +++ b/docker/models/images.py @@ -425,6 +425,7 @@ def pull(self, repository, tag=None, **kwargs): if not tag: repository, tag = parse_repository_tag(repository) + kwargs['stream'] = False self.client.api.pull(repository, tag=tag, **kwargs) if tag: return self.get('{0}{2}{1}'.format( diff --git a/tests/unit/models_containers_test.py b/tests/unit/models_containers_test.py index 22dd241064..957035af0a 100644 --- a/tests/unit/models_containers_test.py +++ b/tests/unit/models_containers_test.py @@ -232,7 +232,8 @@ def test_run_pull(self): container = client.containers.run('alpine', 'sleep 300', detach=True) assert container.id == FAKE_CONTAINER_ID - client.api.pull.assert_called_with('alpine', platform=None, tag=None) + client.api.pull.assert_called_with('alpine', platform=None, tag=None, + stream=False) def test_run_with_error(self): client = make_fake_client() diff --git a/tests/unit/models_images_test.py b/tests/unit/models_images_test.py index 67832795fe..ef81a1599d 100644 --- a/tests/unit/models_images_test.py +++ b/tests/unit/models_images_test.py @@ -43,7 +43,8 @@ def test_load(self): def test_pull(self): client = make_fake_client() image = client.images.pull('test_image:latest') - client.api.pull.assert_called_with('test_image', tag='latest') + client.api.pull.assert_called_with('test_image', tag='latest', + stream=False) client.api.inspect_image.assert_called_with('test_image:latest') assert isinstance(image, Image) assert image.id == FAKE_IMAGE_ID @@ -51,7 +52,8 @@ def test_pull(self): def test_pull_multiple(self): client = make_fake_client() images = client.images.pull('test_image') - client.api.pull.assert_called_with('test_image', tag=None) + client.api.pull.assert_called_with('test_image', tag=None, + stream=False) client.api.images.assert_called_with( all=False, name='test_image', filters=None ) From c53423f11851a69fa18783b5b0b801a24d8d2f82 Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Wed, 28 Nov 2018 11:24:21 -0800 Subject: [PATCH 2/2] Update DockerClient.images.pull to always stream response Also raise a warning when users attempt to specify the "stream" parameter Signed-off-by: Joffrey F --- docker/models/images.py | 18 ++++++++++++++++-- tests/unit/models_containers_test.py | 5 +++-- tests/unit/models_images_test.py | 24 +++++++++++++++++++----- 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/docker/models/images.py b/docker/models/images.py index f8b842a943..30e86f109e 100644 --- a/docker/models/images.py +++ b/docker/models/images.py @@ -1,5 +1,6 @@ import itertools import re +import warnings import six @@ -425,8 +426,21 @@ def pull(self, repository, tag=None, **kwargs): if not tag: repository, tag = parse_repository_tag(repository) - kwargs['stream'] = False - self.client.api.pull(repository, tag=tag, **kwargs) + if 'stream' in kwargs: + warnings.warn( + '`stream` is not a valid parameter for this method' + ' and will be overridden' + ) + del kwargs['stream'] + + pull_log = self.client.api.pull( + repository, tag=tag, stream=True, **kwargs + ) + for _ in pull_log: + # We don't do anything with the logs, but we need + # to keep the connection alive and wait for the image + # to be pulled. + pass if tag: return self.get('{0}{2}{1}'.format( repository, tag, '@' if tag.startswith('sha256:') else ':' diff --git a/tests/unit/models_containers_test.py b/tests/unit/models_containers_test.py index 957035af0a..39e409e4bf 100644 --- a/tests/unit/models_containers_test.py +++ b/tests/unit/models_containers_test.py @@ -232,8 +232,9 @@ def test_run_pull(self): container = client.containers.run('alpine', 'sleep 300', detach=True) assert container.id == FAKE_CONTAINER_ID - client.api.pull.assert_called_with('alpine', platform=None, tag=None, - stream=False) + client.api.pull.assert_called_with( + 'alpine', platform=None, tag=None, stream=True + ) def test_run_with_error(self): client = make_fake_client() diff --git a/tests/unit/models_images_test.py b/tests/unit/models_images_test.py index ef81a1599d..fd894ab71d 100644 --- a/tests/unit/models_images_test.py +++ b/tests/unit/models_images_test.py @@ -1,6 +1,8 @@ +import unittest +import warnings + from docker.constants import DEFAULT_DATA_CHUNK_SIZE from docker.models.images import Image -import unittest from .fake_api import FAKE_IMAGE_ID from .fake_api_client import make_fake_client @@ -43,8 +45,9 @@ def test_load(self): def test_pull(self): client = make_fake_client() image = client.images.pull('test_image:latest') - client.api.pull.assert_called_with('test_image', tag='latest', - stream=False) + client.api.pull.assert_called_with( + 'test_image', tag='latest', stream=True + ) client.api.inspect_image.assert_called_with('test_image:latest') assert isinstance(image, Image) assert image.id == FAKE_IMAGE_ID @@ -52,8 +55,9 @@ def test_pull(self): def test_pull_multiple(self): client = make_fake_client() images = client.images.pull('test_image') - client.api.pull.assert_called_with('test_image', tag=None, - stream=False) + client.api.pull.assert_called_with( + 'test_image', tag=None, stream=True + ) client.api.images.assert_called_with( all=False, name='test_image', filters=None ) @@ -63,6 +67,16 @@ def test_pull_multiple(self): assert isinstance(image, Image) assert image.id == FAKE_IMAGE_ID + def test_pull_with_stream_param(self): + client = make_fake_client() + with warnings.catch_warnings(record=True) as w: + client.images.pull('test_image', stream=True) + + assert len(w) == 1 + assert str(w[0].message).startswith( + '`stream` is not a valid parameter' + ) + def test_push(self): client = make_fake_client() client.images.push('foobar', insecure_registry=True)