attach_volume should always return a vol attachment.

attach_volume method has inconsistent return value: if we pass it
wait=True, the methods returns a volume object but if we pass it
wait=False it returns a volume_attachment object.

We should always return a volume_attachment object.

Also don't try to be too clever in pre-emptively refusing to detach
a volume that we think is not attached. It prevents the following
calls, which looks good without knowing Shade internals:
server = create_server(...); volume = create_volume(...);
attach_volume(server, volume); detach_volume(server, volume)

Change-Id: Ia1da29ec6286dbbed0a77d6abcf89e95a055ac9a

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.
+

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))

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(

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):