From e5df7aba69564be114c3a236bc73a3150555bc49 Mon Sep 17 00:00:00 2001 From: Mitsuhiro Tanino Date: Mon, 7 Dec 2015 17:05:24 -0500 Subject: [PATCH] Add validation for volume_type of volume object During copy_volume_to_image(), this tries to return volume['volume_type'] attribute in a volume object, but if the volume does not have a volume type, this would trigger a db call which raise an 404 not found error. Volume object should check volume_type_id at first and then decide whether returning volume_type or None to avoid VolumeTypeNotFound exception. Also this change includes LOG.info messages fix to show right behavior of copy_volume_to_image(). Change-Id: Id2372621962406282eece2bcbc56c242f9dff8a0 Closes-Bug: #1523362 --- cinder/objects/volume.py | 7 ++- .../unit/api/contrib/test_volume_actions.py | 46 +++++++++++++++++++ cinder/tests/unit/objects/test_volume.py | 14 ++++-- cinder/volume/api.py | 2 +- 4 files changed, 63 insertions(+), 6 deletions(-) diff --git a/cinder/objects/volume.py b/cinder/objects/volume.py index a423f9191..70533c54d 100644 --- a/cinder/objects/volume.py +++ b/cinder/objects/volume.py @@ -290,8 +290,11 @@ class Volume(base.CinderPersistentObject, base.CinderObject, self.glance_metadata = db.volume_glance_metadata_get( self._context, self.id) elif attrname == 'volume_type': - self.volume_type = objects.VolumeType.get_by_id( - self._context, self.volume_type_id) + # If the volume doesn't have volume_type, VolumeType.get_by_id + # would trigger a db call which raise VolumeTypeNotFound exception. + self.volume_type = (objects.VolumeType.get_by_id( + self._context, self.volume_type_id) if self.volume_type_id + else None) elif attrname == 'volume_attachment': attachments = objects.VolumeAttachmentList.get_all_by_volume_id( self._context, self.id) diff --git a/cinder/tests/unit/api/contrib/test_volume_actions.py b/cinder/tests/unit/api/contrib/test_volume_actions.py index 0a72048c6..c34da3b9b 100644 --- a/cinder/tests/unit/api/contrib/test_volume_actions.py +++ b/cinder/tests/unit/api/contrib/test_volume_actions.py @@ -27,6 +27,7 @@ from cinder.api.contrib import volume_actions from cinder import context from cinder import exception from cinder.image import glance +from cinder import objects from cinder import test from cinder.tests.unit.api import fakes from cinder.tests.unit.api.v2 import stubs @@ -562,6 +563,7 @@ class VolumeImageActionsTest(test.TestCase): self.controller = volume_actions.VolumeActionsController() self.stubs.Set(volume_api.API, 'get', stub_volume_get) + self.context = context.RequestContext('fake', 'fake', is_admin=False) def _get_os_volume_upload_image(self): vol = { @@ -967,3 +969,47 @@ class VolumeImageActionsTest(test.TestCase): } self.assertDictMatch(expected_res, res_dict) + + @mock.patch.object(volume_api.API, "get_volume_image_metadata") + @mock.patch.object(glance.GlanceImageService, "create") + @mock.patch.object(volume_api.API, "get") + @mock.patch.object(volume_api.API, "update") + @mock.patch.object(volume_rpcapi.VolumeAPI, "copy_volume_to_image") + def test_copy_volume_to_image_volume_type_none( + self, + mock_copy_volume_to_image, + mock_update, + mock_get, + mock_create, + mock_get_volume_image_metadata): + """Test create image from volume with none type volume.""" + + db_volume = fake_volume.fake_db_volume() + volume_obj = objects.Volume._from_db_object(self.context, + objects.Volume(), + db_volume) + + mock_create.side_effect = self.fake_image_service_create + mock_get.return_value = volume_obj + mock_copy_volume_to_image.side_effect = ( + self.fake_rpc_copy_volume_to_image) + + req = fakes.HTTPRequest.blank('/v2/tenant1/volumes/%s/action' % id) + body = self._get_os_volume_upload_image() + res_dict = self.controller._volume_upload_image(req, id, body) + expected_res = { + 'os-volume_upload_image': { + 'id': '1', + 'updated_at': None, + 'status': 'uploading', + 'display_description': None, + 'size': 1, + 'volume_type': None, + 'image_id': 1, + 'container_format': u'bare', + 'disk_format': u'raw', + 'image_name': u'image_name' + } + } + + self.assertDictMatch(expected_res, res_dict) diff --git a/cinder/tests/unit/objects/test_volume.py b/cinder/tests/unit/objects/test_volume.py index 2875af8b9..aa199e17a 100644 --- a/cinder/tests/unit/objects/test_volume.py +++ b/cinder/tests/unit/objects/test_volume.py @@ -187,11 +187,19 @@ class TestVolume(test_objects.BaseObjectsTestCase): self.context, volume.id) # Test volume_type lazy-loaded field - volume_type = objects.VolumeType(context=self.context, id=5) + # Case1. volume.volume_type_id = None + self.assertIsNone(volume.volume_type) + + # Case2. volume2.volume_type_id = 1 + fake2 = fake_volume.fake_db_volume() + fake2.update({'volume_type_id': 1}) + volume2 = objects.Volume._from_db_object( + self.context, objects.Volume(), fake2) + volume_type = objects.VolumeType(context=self.context, id=1) mock_vt_get_by_id.return_value = volume_type - self.assertEqual(volume_type, volume.volume_type) + self.assertEqual(volume_type, volume2.volume_type) mock_vt_get_by_id.assert_called_once_with(self.context, - volume.volume_type_id) + volume2.volume_type_id) # Test consistencygroup lazy-loaded field consistencygroup = objects.ConsistencyGroup(context=self.context, id=2) diff --git a/cinder/volume/api.py b/cinder/volume/api.py index ac01271a3..fa03788df 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -1217,7 +1217,7 @@ class API(base.Base): "container_format": recv_metadata['container_format'], "disk_format": recv_metadata['disk_format'], "image_name": recv_metadata.get('name', None)} - LOG.info(_LI("Copy image to volume completed successfully."), + LOG.info(_LI("Copy volume to image completed successfully."), resource=volume) return response -- 2.45.2