Chunked GET request support
In one test, we are downloading the entire image (into memory) and
re-uploading it. That works when the image is 16MiB but not when it
is 1GiB. This adds support to the internal http client for chunked
downloads (similar to upload), makes the image client able to take
that flag, and finally makes the offending test do a chunked upload/
download streaming operation.
Note this un-skips the test, effectively reverting a6b7e334c
because the test should no longer consume large amounts of memory.
Related-Bug: #2002951
Change-Id: I31e537538a1862e71091aa470da3b8e9c799bf15
diff --git a/tempest/api/compute/admin/test_volume.py b/tempest/api/compute/admin/test_volume.py
index 2fcd053..e7c931e 100644
--- a/tempest/api/compute/admin/test_volume.py
+++ b/tempest/api/compute/admin/test_volume.py
@@ -13,8 +13,6 @@
# License for the specific language governing permissions and limitations
# under the License.
-import io
-
from tempest.api.compute import base
from tempest.common import waiters
from tempest import config
@@ -49,9 +47,11 @@
:param return image_id: The UUID of the newly created image.
"""
image = self.admin_image_client.show_image(CONF.compute.image_ref)
- image_data = self.admin_image_client.show_image_file(
- CONF.compute.image_ref).data
- image_file = io.BytesIO(image_data)
+ # NOTE(danms): We need to stream this, so chunked=True means we get
+ # back a urllib3.HTTPResponse and have to carefully pass it to
+ # store_image_file() to upload it in pieces.
+ image_data_resp = self.admin_image_client.show_image_file(
+ CONF.compute.image_ref, chunked=True)
create_dict = {
'container_format': image['container_format'],
'disk_format': image['disk_format'],
@@ -60,24 +60,22 @@
'visibility': 'public',
}
create_dict.update(kwargs)
- new_image = self.admin_image_client.create_image(**create_dict)
- self.addCleanup(self.admin_image_client.wait_for_resource_deletion,
- new_image['id'])
- self.addCleanup(self.admin_image_client.delete_image, new_image['id'])
- self.admin_image_client.store_image_file(new_image['id'], image_file)
-
+ try:
+ new_image = self.admin_image_client.create_image(**create_dict)
+ self.addCleanup(self.admin_image_client.wait_for_resource_deletion,
+ new_image['id'])
+ self.addCleanup(
+ self.admin_image_client.delete_image, new_image['id'])
+ self.admin_image_client.store_image_file(new_image['id'],
+ image_data_resp)
+ finally:
+ image_data_resp.release_conn()
return new_image['id']
class AttachSCSIVolumeTestJSON(BaseAttachSCSIVolumeTest):
"""Test attaching scsi volume to server"""
- # NOTE(gibi): https://bugs.launchpad.net/nova/+bug/2002951/comments/5 shows
- # that calling _create_image_with_custom_property can cause excessive
- # memory usage in the test executor as it downloads a glance image in
- # memory. This is causing gate failures so the test is disabled. One
- # potential fix is to do a chunked data download / upload loop instead.
- @decorators.skip_because(bug="2002951", condition=True)
@decorators.idempotent_id('777e468f-17ca-4da4-b93d-b7dbf56c0494')
def test_attach_scsi_disk_with_config_drive(self):
"""Test the attach/detach volume with config drive/scsi disk
diff --git a/tempest/lib/common/http.py b/tempest/lib/common/http.py
index 33f871b..d163968 100644
--- a/tempest/lib/common/http.py
+++ b/tempest/lib/common/http.py
@@ -60,7 +60,12 @@
retry = urllib3.util.Retry(redirect=False)
r = super(ClosingProxyHttp, self).request(method, url, retries=retry,
*args, **new_kwargs)
- return Response(r), r.data
+ if not kwargs.get('preload_content', True):
+ # This means we asked urllib3 for streaming content, so we
+ # need to return the raw response and not read any data yet
+ return r, b''
+ else:
+ return Response(r), r.data
class ClosingHttp(urllib3.poolmanager.PoolManager):
@@ -109,4 +114,9 @@
retry = urllib3.util.Retry(redirect=False)
r = super(ClosingHttp, self).request(method, url, retries=retry,
*args, **new_kwargs)
- return Response(r), r.data
+ if not kwargs.get('preload_content', True):
+ # This means we asked urllib3 for streaming content, so we
+ # need to return the raw response and not read any data yet
+ return r, b''
+ else:
+ return Response(r), r.data
diff --git a/tempest/lib/common/rest_client.py b/tempest/lib/common/rest_client.py
index a11b7c1..6cf5b73 100644
--- a/tempest/lib/common/rest_client.py
+++ b/tempest/lib/common/rest_client.py
@@ -19,6 +19,7 @@
import re
import time
import urllib
+import urllib3
import jsonschema
from oslo_log import log as logging
@@ -298,7 +299,7 @@
"""
return self.request('POST', url, extra_headers, headers, body, chunked)
- def get(self, url, headers=None, extra_headers=False):
+ def get(self, url, headers=None, extra_headers=False, chunked=False):
"""Send a HTTP GET request using keystone service catalog and auth
:param str url: the relative url to send the get request to
@@ -307,11 +308,19 @@
returned by the get_headers() method are to
be used but additional headers are needed in
the request pass them in as a dict.
+ :param bool chunked: Boolean value that indicates if we should stream
+ the response instead of reading it all at once.
+ If True, data will be empty and the raw urllib3
+ response object will be returned.
+ NB: If you pass True here, you **MUST** call
+ release_conn() on the response object before
+ finishing!
:return: a tuple with the first entry containing the response headers
and the second the response body
:rtype: tuple
"""
- return self.request('GET', url, extra_headers, headers)
+ return self.request('GET', url, extra_headers, headers,
+ chunked=chunked)
def delete(self, url, headers=None, body=None, extra_headers=False):
"""Send a HTTP DELETE request using keystone service catalog and auth
@@ -480,7 +489,7 @@
self.LOG.info(
'Request (%s): %s %s %s%s',
caller_name,
- resp['status'],
+ resp.status,
method,
req_url,
secs,
@@ -617,17 +626,30 @@
"""
if headers is None:
headers = self.get_headers()
+ # In urllib3, chunked only affects the upload. However, we may
+ # want to read large responses to GET incrementally. Re-purpose
+ # chunked=True on a GET to also control how we handle the response.
+ preload = not (method.lower() == 'get' and chunked)
+ if not preload:
+ # NOTE(danms): Not specifically necessary, but don't send
+ # chunked=True to urllib3 on a GET, since it is technically
+ # for PUT/POST type operations
+ chunked = False
# Do the actual request, and time it
start = time.time()
self._log_request_start(method, url)
resp, resp_body = self.http_obj.request(
url, method, headers=headers,
- body=body, chunked=chunked)
+ body=body, chunked=chunked, preload_content=preload)
end = time.time()
req_body = body if log_req_body is None else log_req_body
- self._log_request(method, url, resp, secs=(end - start),
- req_headers=headers, req_body=req_body,
- resp_body=resp_body)
+ if preload:
+ # NOTE(danms): If we are reading the whole response, we can do
+ # this logging. If not, skip the logging because it will result
+ # in us reading the response data prematurely.
+ self._log_request(method, url, resp, secs=(end - start),
+ req_headers=headers, req_body=req_body,
+ resp_body=resp_body)
return resp, resp_body
def request(self, method, url, extra_headers=False, headers=None,
@@ -773,6 +795,10 @@
# resp this could possibly fail
if str(type(resp)) == "<type 'instance'>":
ctype = resp.getheader('content-type')
+ elif isinstance(resp, urllib3.HTTPResponse):
+ # If we requested chunked=True streaming, this will be a raw
+ # urllib3.HTTPResponse
+ ctype = resp.getheaders()['content-type']
else:
try:
ctype = resp['content-type']
diff --git a/tempest/lib/services/image/v2/images_client.py b/tempest/lib/services/image/v2/images_client.py
index ae6ce25..8460b57 100644
--- a/tempest/lib/services/image/v2/images_client.py
+++ b/tempest/lib/services/image/v2/images_client.py
@@ -248,17 +248,26 @@
self.expected_success(202, resp.status)
return rest_client.ResponseBody(resp)
- def show_image_file(self, image_id):
+ def show_image_file(self, image_id, chunked=False):
"""Download binary image data.
+ :param bool chunked: If True, do not read the body and return only
+ the raw urllib3 response object for processing.
+ NB: If you pass True here, you **MUST** call
+ release_conn() on the response object before
+ finishing!
+
For a full list of available parameters, please refer to the official
API reference:
https://docs.openstack.org/api-ref/image/v2/#download-binary-image-data
"""
url = 'images/%s/file' % image_id
- resp, body = self.get(url)
+ resp, body = self.get(url, chunked=chunked)
self.expected_success([200, 204, 206], resp.status)
- return rest_client.ResponseBodyData(resp, body)
+ if chunked:
+ return resp
+ else:
+ return rest_client.ResponseBodyData(resp, body)
def add_image_tag(self, image_id, tag):
"""Add an image tag.
diff --git a/tempest/tests/lib/common/test_http.py b/tempest/tests/lib/common/test_http.py
index a19153f..aae6ba2 100644
--- a/tempest/tests/lib/common/test_http.py
+++ b/tempest/tests/lib/common/test_http.py
@@ -149,6 +149,31 @@
'xtra key': 'Xtra Value'},
response)
+ def test_request_preload(self):
+ # Given
+ connection = self.closing_http()
+ headers = {'Xtra Key': 'Xtra Value'}
+ http_response = urllib3.HTTPResponse(headers=headers)
+ request = self.patch('urllib3.PoolManager.request',
+ return_value=http_response)
+ retry = self.patch('urllib3.util.Retry')
+
+ # When
+ response, _ = connection.request(
+ method=REQUEST_METHOD,
+ url=REQUEST_URL,
+ headers=headers,
+ preload_content=False)
+
+ # Then
+ request.assert_called_once_with(
+ REQUEST_METHOD,
+ REQUEST_URL,
+ headers=dict(headers, connection='close'),
+ preload_content=False,
+ retries=retry(raise_on_redirect=False, redirect=5))
+ self.assertIsInstance(response, urllib3.HTTPResponse)
+
class TestClosingProxyHttp(TestClosingHttp):
diff --git a/tempest/tests/lib/common/test_rest_client.py b/tempest/tests/lib/common/test_rest_client.py
index 910756f..81a76e0 100644
--- a/tempest/tests/lib/common/test_rest_client.py
+++ b/tempest/tests/lib/common/test_rest_client.py
@@ -55,6 +55,7 @@
def test_get(self):
__, return_dict = self.rest_client.get(self.url)
self.assertEqual('GET', return_dict['method'])
+ self.assertTrue(return_dict['preload_content'])
def test_delete(self):
__, return_dict = self.rest_client.delete(self.url)
@@ -78,6 +79,17 @@
__, return_dict = self.rest_client.copy(self.url)
self.assertEqual('COPY', return_dict['method'])
+ def test_get_chunked(self):
+ self.useFixture(fixtures.MockPatchObject(self.rest_client,
+ '_log_request'))
+ __, return_dict = self.rest_client.get(self.url, chunked=True)
+ # Default is preload_content=True, make sure we passed False
+ self.assertFalse(return_dict['preload_content'])
+ # Make sure we did not pass chunked=True to urllib3 for GET
+ self.assertFalse(return_dict['chunked'])
+ # Make sure we did not call _log_request() on the raw response
+ self.rest_client._log_request.assert_not_called()
+
class TestRestClientNotFoundHandling(BaseRestClientTestClass):
def setUp(self):
diff --git a/tempest/tests/lib/fake_http.py b/tempest/tests/lib/fake_http.py
index cfa4b93..5fa0c43 100644
--- a/tempest/tests/lib/fake_http.py
+++ b/tempest/tests/lib/fake_http.py
@@ -21,14 +21,17 @@
self.return_type = return_type
def request(self, uri, method="GET", body=None, headers=None,
- redirections=5, connection_type=None, chunked=False):
+ redirections=5, connection_type=None, chunked=False,
+ preload_content=False):
if not self.return_type:
fake_headers = fake_http_response(headers)
return_obj = {
'uri': uri,
'method': method,
'body': body,
- 'headers': headers
+ 'headers': headers,
+ 'chunked': chunked,
+ 'preload_content': preload_content,
}
return (fake_headers, return_obj)
elif isinstance(self.return_type, int):
diff --git a/tempest/tests/lib/services/image/v2/test_images_client.py b/tempest/tests/lib/services/image/v2/test_images_client.py
index 5b162f8..27a50a9 100644
--- a/tempest/tests/lib/services/image/v2/test_images_client.py
+++ b/tempest/tests/lib/services/image/v2/test_images_client.py
@@ -13,6 +13,9 @@
# under the License.
import io
+from unittest import mock
+
+import fixtures
from tempest.lib.common.utils import data_utils
from tempest.lib.services.image.v2 import images_client
@@ -239,6 +242,21 @@
headers={'Content-Type': 'application/octet-stream'},
status=200)
+ def test_show_image_file_chunked(self):
+ # Since chunked=True on a GET should pass the response object
+ # basically untouched, we use a mock here so we get some assurances.
+ http_response = mock.MagicMock()
+ http_response.status = 200
+ self.useFixture(fixtures.MockPatch(
+ 'tempest.lib.common.rest_client.RestClient.get',
+ return_value=(http_response, b'')))
+ resp = self.client.show_image_file(
+ self.FAKE_CREATE_UPDATE_SHOW_IMAGE['id'],
+ chunked=True)
+ self.assertEqual(http_response, resp)
+ resp.__contains__.assert_not_called()
+ resp.__getitem__.assert_not_called()
+
def test_add_image_tag(self):
self.check_service_client_function(
self.client.add_image_tag,