diff --git a/tempest/api/compute/admin/test_volume.py b/tempest/api/compute/admin/test_volume.py index 2fcd05382c..e7c931ed09 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 @@ class BaseAttachSCSIVolumeTest(base.BaseV2ComputeAdminTest): :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 @@ class BaseAttachSCSIVolumeTest(base.BaseV2ComputeAdminTest): '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 33f871bc74..d16396856f 100644 --- a/tempest/lib/common/http.py +++ b/tempest/lib/common/http.py @@ -60,7 +60,12 @@ class ClosingProxyHttp(urllib3.ProxyManager): 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 @@ class ClosingHttp(urllib3.poolmanager.PoolManager): 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 a11b7c1335..6cf5b734cb 100644 --- a/tempest/lib/common/rest_client.py +++ b/tempest/lib/common/rest_client.py @@ -19,6 +19,7 @@ import email.utils import re import time import urllib +import urllib3 import jsonschema from oslo_log import log as logging @@ -298,7 +299,7 @@ class RestClient(object): """ 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 @@ class RestClient(object): 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 @@ class RestClient(object): self.LOG.info( 'Request (%s): %s %s %s%s', caller_name, - resp['status'], + resp.status, method, req_url, secs, @@ -617,17 +626,30 @@ class RestClient(object): """ 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 @@ class RestClient(object): # resp this could possibly fail if str(type(resp)) == "": 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 ae6ce25ded..8460b57c43 100644 --- a/tempest/lib/services/image/v2/images_client.py +++ b/tempest/lib/services/image/v2/images_client.py @@ -248,17 +248,26 @@ class ImagesClient(rest_client.RestClient): 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 a19153f0e9..aae6ba2d71 100644 --- a/tempest/tests/lib/common/test_http.py +++ b/tempest/tests/lib/common/test_http.py @@ -149,6 +149,31 @@ class TestClosingHttp(base.TestCase): '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 910756facd..81a76e02c2 100644 --- a/tempest/tests/lib/common/test_rest_client.py +++ b/tempest/tests/lib/common/test_rest_client.py @@ -55,6 +55,7 @@ class TestRestClientHTTPMethods(BaseRestClientTestClass): 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 @@ class TestRestClientHTTPMethods(BaseRestClientTestClass): __, 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 cfa4b93685..5fa0c4338f 100644 --- a/tempest/tests/lib/fake_http.py +++ b/tempest/tests/lib/fake_http.py @@ -21,14 +21,17 @@ class fake_httplib2(object): 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 5b162f83c3..27a50a95f5 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 @@ class TestImagesClient(base.BaseServiceTest): 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,