diff --git a/releasenotes/notes/change-attach-vol-return-value-4834a1f78392abb1.yaml b/releasenotes/notes/change-attach-vol-return-value-4834a1f78392abb1.yaml new file mode 100644 index 000000000..19db8ebef --- /dev/null +++ b/releasenotes/notes/change-attach-vol-return-value-4834a1f78392abb1.yaml @@ -0,0 +1,8 @@ +--- +upgrade: + - | + The ``attach_volume`` method now always returns a ``volume_attachment`` + object. Previously, ``attach_volume`` would return a ``volume`` object if + it was called with ``wait=True`` and a ``volume_attachment`` object + otherwise. + diff --git a/shade/openstackcloud.py b/shade/openstackcloud.py index 781381b64..abcc3fb02 100644 --- a/shade/openstackcloud.py +++ b/shade/openstackcloud.py @@ -3856,12 +3856,6 @@ class OpenStackCloud(_normalize.Normalizer): :raises: OpenStackCloudTimeout if wait time exceeded. :raises: OpenStackCloudException on operation error. """ - dev = self.get_volume_attach_device(volume, server['id']) - if not dev: - raise OpenStackCloudException( - "Volume %s is not attached to server %s" - % (volume['id'], server['id']) - ) with _utils.shade_exceptions( "Error detaching volume {volume} from server {server}".format( @@ -3909,6 +3903,8 @@ class OpenStackCloud(_normalize.Normalizer): :param wait: If true, waits for volume to be attached. :param timeout: Seconds to wait for volume attachment. None is forever. + :returns: a volume attachment object. + :raises: OpenStackCloudTimeout if wait time exceeded. :raises: OpenStackCloudException on operation error. """ @@ -3929,7 +3925,7 @@ class OpenStackCloud(_normalize.Normalizer): "Error attaching volume {volume_id} to server " "{server_id}".format(volume_id=volume['id'], server_id=server['id'])): - vol = self.manager.submit_task( + vol_attachment = self.manager.submit_task( _tasks.VolumeAttach(volume_id=volume['id'], server_id=server['id'], device=device)) @@ -3957,7 +3953,7 @@ class OpenStackCloud(_normalize.Normalizer): raise OpenStackCloudException( "Error in attaching volume %s" % volume['id'] ) - return vol + return vol_attachment def _get_volume_kwargs(self, kwargs): name = kwargs.pop('name', kwargs.pop('display_name', None)) diff --git a/shade/tests/functional/test_compute.py b/shade/tests/functional/test_compute.py index 754b47874..ef4cacdcb 100644 --- a/shade/tests/functional/test_compute.py +++ b/shade/tests/functional/test_compute.py @@ -27,6 +27,10 @@ from shade import _utils class TestCompute(base.BaseFunctionalTestCase): def setUp(self): + # OS_TEST_TIMEOUT is 60 sec by default + # but on a bad day, test_attach_detach_volume can take more time. + self.TIMEOUT_SCALING_FACTOR = 1.5 + super(TestCompute, self).setUp() self.flavor = pick_flavor(self.user_cloud.list_flavors()) if self.flavor is None: @@ -66,6 +70,19 @@ class TestCompute(base.BaseFunctionalTestCase): self.user_cloud.delete_server(self.server_name, wait=True)) self.assertIsNone(self.user_cloud.get_server(self.server_name)) + def test_attach_detach_volume(self): + server_name = self.getUniqueString() + self.addCleanup(self._cleanup_servers_and_volumes, server_name) + server = self.user_cloud.create_server( + name=server_name, image=self.image, flavor=self.flavor, + wait=True) + volume = self.user_cloud.create_volume(1) + vol_attachment = self.user_cloud.attach_volume(server, volume) + for key in ('device', 'serverId', 'volumeId'): + self.assertIn(key, vol_attachment) + self.assertTrue(vol_attachment[key]) # assert string is not empty + self.assertIsNone(self.user_cloud.detach_volume(server, volume)) + def test_list_all_servers(self): self.addCleanup(self._cleanup_servers_and_volumes, self.server_name) server = self.user_cloud.create_server( diff --git a/shade/tests/unit/test_volume.py b/shade/tests/unit/test_volume.py index 7f0b2d175..e5a410105 100644 --- a/shade/tests/unit/test_volume.py +++ b/shade/tests/unit/test_volume.py @@ -25,15 +25,13 @@ class TestVolume(base.TestCase): def test_attach_volume(self, mock_nova): server = dict(id='server001') volume = dict(id='volume001', status='available', attachments=[]) - rvol = dict(id='volume001', status='attached', - attachments=[ - {'server_id': server['id'], 'device': 'device001'} - ]) - mock_nova.volumes.create_server_volume.return_value = rvol + rattach = {'server_id': server['id'], 'device': 'device001', + 'volumeId': volume['id'], 'id': 'attachmentId'} + mock_nova.volumes.create_server_volume.return_value = rattach ret = self.cloud.attach_volume(server, volume, wait=False) - self.assertEqual(rvol, ret) + self.assertEqual(rattach, ret) mock_nova.volumes.create_server_volume.assert_called_once_with( volume_id=volume['id'], server_id=server['id'], device=None ) @@ -60,6 +58,7 @@ class TestVolume(base.TestCase): id=volume['id'], status='attached', attachments=[{'server_id': server['id'], 'device': 'device001'}] ) + mock_get.side_effect = iter([volume, attached_volume]) # defaults to wait=True @@ -69,7 +68,8 @@ class TestVolume(base.TestCase): volume_id=volume['id'], server_id=server['id'], device=None ) self.assertEqual(2, mock_get.call_count) - self.assertEqual(attached_volume, ret) + self.assertEqual(mock_nova.volumes.create_server_volume.return_value, + ret) @mock.patch.object(shade.OpenStackCloud, 'get_volume') @mock.patch.object(shade.OpenStackCloud, 'nova_client') @@ -140,20 +140,6 @@ class TestVolume(base.TestCase): ): self.cloud.detach_volume(server, volume, wait=False) - @mock.patch.object(shade.OpenStackCloud, 'nova_client') - def test_detach_volume_not_attached(self, mock_nova): - server = dict(id='server001') - volume = dict(id='volume001', - attachments=[ - {'server_id': 'server999', 'device': 'device001'} - ]) - with testtools.ExpectedException( - shade.OpenStackCloudException, - "Volume %s is not attached to server %s" % ( - volume['id'], server['id']) - ): - self.cloud.detach_volume(server, volume, wait=False) - @mock.patch.object(shade.OpenStackCloud, 'get_volume') @mock.patch.object(shade.OpenStackCloud, 'nova_client') def test_detach_volume_wait(self, mock_nova, mock_get):