From 181be53a2b0a54f7b4cd33dacbaa560e124e9421 Mon Sep 17 00:00:00 2001 From: Tomoki Sekiyama Date: Fri, 27 Mar 2015 15:35:05 -0400 Subject: [PATCH] Fix volume creation from image with allowed_direct_url_schemes When CONF.allowed_direct_url_schemes is set to ['file'] and Glance image has direct_url or locations metadata with 'file:///' scheme, cinder tries to directly read the image data from the local file. However, currently the code is broken because return value from get_location() has been changed to a tuple. This fixes the code so that it can handle the metadata correctly. Change-Id: I39a12a31fbfbd3a9824c67391096f74406d8a749 Closes-Bug: #1437477 --- cinder/image/glance.py | 23 ++++++++++++++--------- cinder/tests/unit/image/test_glance.py | 25 +++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/cinder/image/glance.py b/cinder/image/glance.py index 2a248d9bf1b..ecc3314c6f7 100644 --- a/cinder/image/glance.py +++ b/cinder/image/glance.py @@ -242,8 +242,9 @@ class GlanceImageService(object): return base_image_meta def get_location(self, context, image_id): - """Returns the direct url representing the backend storage location, - or None if this attribute is not shown by Glance. + """Returns a tuple of the direct url and locations representing the + backend storage location, or (None, None) if these attributes are not + shown by Glance. """ if CONF.glance_api_version == 1: # image location not available in v1 @@ -266,16 +267,20 @@ class GlanceImageService(object): def download(self, context, image_id, data=None): """Calls out to Glance for data and writes data.""" - if 'file' in CONF.allowed_direct_url_schemes: - location = self.get_location(context, image_id) - o = urlparse.urlparse(location) - if o.scheme == "file": - with open(o.path, "r") as f: + if data and 'file' in CONF.allowed_direct_url_schemes: + direct_url, locations = self.get_location(context, image_id) + urls = [direct_url] + [loc.get('url') for loc in locations or []] + for url in urls: + if url is None: + continue + parsed_url = urlparse.urlparse(url) + if parsed_url.scheme == "file": # a system call to cp could have significant performance # advantages, however we do not have the path to files at # this point in the abstraction. - shutil.copyfileobj(f, data) - return + with open(parsed_url.path, "r") as f: + shutil.copyfileobj(f, data) + return try: image_chunks = self._client.call(context, 'data', image_id) diff --git a/cinder/tests/unit/image/test_glance.py b/cinder/tests/unit/image/test_glance.py index f4032c7bf6a..8f043e32f08 100644 --- a/cinder/tests/unit/image/test_glance.py +++ b/cinder/tests/unit/image/test_glance.py @@ -518,6 +518,31 @@ class TestGlanceImageService(test.TestCase): self.assertRaises(exception.ImageNotFound, service.download, self.context, image_id, writer) + @mock.patch('__builtin__.open') + @mock.patch('shutil.copyfileobj') + def test_download_from_direct_file(self, mock_copyfileobj, mock_open): + fixture = self._make_fixture(name='test image', + locations=[{'url': 'file:///tmp/test'}]) + image_id = self.service.create(self.context, fixture)['id'] + writer = NullWriter() + self.flags(allowed_direct_url_schemes=['file']) + self.flags(glance_api_version=2) + self.service.download(self.context, image_id, writer) + mock_copyfileobj.assert_called_once_with(mock.ANY, writer) + + @mock.patch('__builtin__.open') + @mock.patch('shutil.copyfileobj') + def test_download_from_direct_file_non_file(self, + mock_copyfileobj, mock_open): + fixture = self._make_fixture(name='test image', + direct_url='swift+http://test/image') + image_id = self.service.create(self.context, fixture)['id'] + writer = NullWriter() + self.flags(allowed_direct_url_schemes=['file']) + self.flags(glance_api_version=2) + self.service.download(self.context, image_id, writer) + self.assertEqual(None, mock_copyfileobj.call_args) + def test_glance_client_image_id(self): fixture = self._make_fixture(name='test image') image_id = self.service.create(self.context, fixture)['id']