Fixed timeout logic when attaching/detaching volumes.

The timeout logic in nova_utils.attach_volume() and detach_volume()
was not correct which may have been the root cause behind the issue
FUNCTEST-927. Timeout in both attach and detach is no longer
optional.

Also added a test to attach and detach without timeout as that path
was never tested. Updated associated test documentation as well.

JIRA: SNAPS-263
JIRA: FUNCTEST-927

Change-Id: Iea3aeab59c378917fbd175d673113e8d30e2e4b9
Signed-off-by: spisarski <s.pisarski@cablelabs.com>

4 files changed, 149 insertions, 49 deletions

diff --git a/docs/how-to-use/APITests.rst b/docs/how-to-use/APITests.rst
index 50cd437..6239a08 100644
--- a/docs/how-to-use/APITests.rst
+++ b/docs/how-to-use/APITests.rst
@@ -435,7 +435,16 @@ nova_utils_tests.py - NovaUtilsInstanceVolumeTests
 | Test Name                             | Nova API      | Description                                               |
 +=======================================+===============+===========================================================+
 | test_add_remove_volume                | 2             | Ensures that a VM instance can properly attach and detach |
-|                                       |               | a volume using the nova interface                         |
+|                                       |               | a volume using the nova interface while waiting for       |
+|                                       |               | the update to fully occur                                 |
++---------------------------------------+---------------+-----------------------------------------------------------+
+| test_attach_volume_nowait             | 2             | Ensures that the call to nova_utils.attach_volume raises  |
+|                                       |               | an exception when the timeout is too short to return an   |
+|                                       |               | properly updated VmInst object                            |
++---------------------------------------+---------------+-----------------------------------------------------------+
+| test_detach_volume_nowait             | 2             | Ensures that the call to nova_utils.detach_volume raises  |
+|                                       |               | an exception when the timeout is too short to return an   |
+|                                       |               | properly updated VmInst object                            |
 +---------------------------------------+---------------+-----------------------------------------------------------+
 
 create_flavor_tests.py - CreateFlavorTests
diff --git a/snaps/openstack/create_instance.py b/snaps/openstack/create_instance.py
index cdc778f..73cdf8a 100644
--- a/snaps/openstack/create_instance.py
+++ b/snaps/openstack/create_instance.py
@@ -30,6 +30,7 @@ __author__ = 'spisarski'
 logger = logging.getLogger('create_instance')
 
 POLL_INTERVAL = 3
+VOL_DETACH_TIMEOUT = 120
 STATUS_ACTIVE = 'ACTIVE'
 STATUS_DELETED = 'DELETED'
 
@@ -164,15 +165,15 @@ class OpenStackVmInstance(OpenStackComputeObject):
                     cinder, volume_name=volume_name)
 
                 if volume and self.vm_active(block=True):
-                    timeout = 30
                     vm = nova_utils.attach_volume(
-                        self._nova, self.__neutron, self.__vm, volume, timeout)
+                        self._nova, self.__neutron, self.__vm, volume,
+                        VOL_DETACH_TIMEOUT)
 
                     if vm:
                         self.__vm = vm
                     else:
                         logger.warn('Volume [%s] not attached within timeout '
-                                    'of [%s]', volume.name, timeout)
+                                    'of [%s]', volume.name, VOL_DETACH_TIMEOUT)
                 else:
                     logger.warn('Unable to attach volume named [%s]',
                                 volume_name)
@@ -271,7 +272,8 @@ class OpenStackVmInstance(OpenStackComputeObject):
                     cinder, volume_rec['id'])
                 if volume:
                     vm = nova_utils.detach_volume(
-                        self._nova, self.__neutron, self.__vm, volume, 30)
+                        self._nova, self.__neutron, self.__vm, volume,
+                        VOL_DETACH_TIMEOUT)
                     if vm:
                         self.__vm = vm
                     else:
diff --git a/snaps/openstack/utils/nova_utils.py b/snaps/openstack/utils/nova_utils.py
index a8e051e..ba902f9 100644
--- a/snaps/openstack/utils/nova_utils.py
+++ b/snaps/openstack/utils/nova_utils.py
@@ -35,6 +35,8 @@ __author__ = 'spisarski'
 
 logger = logging.getLogger('nova_utils')
 
+POLL_INTERVAL = 3
+
 """
 Utilities for basic OpenStack Nova API calls
 """
@@ -711,60 +713,65 @@ def update_quotas(nova, project_id, compute_quotas):
     return nova.quotas.update(project_id, **update_values)
 
 
-def attach_volume(nova, neutron, server, volume, timeout=None):
+def attach_volume(nova, neutron, server, volume, timeout=120):
     """
-    Attaches a volume to a server
+    Attaches a volume to a server. When the timeout parameter is used, a VmInst
+    object with the proper volume updates is returned unless it has not been
+    updated in the allotted amount of time then an Exception will be raised.
     :param nova: the nova client
     :param neutron: the neutron client
     :param server: the VMInst domain object
     :param volume: the Volume domain object
     :param timeout: denotes the amount of time to block to determine if the
-                    has been properly attached. When None, do not wait.
-    :return: the value from the nova call
+                    has been properly attached.
+    :return: updated VmInst object
     """
     nova.volumes.create_server_volume(server.id, volume.id)
 
-    if timeout:
-        start_time = time.time()
-        while time.time() < start_time + timeout:
-            vm = get_server_object_by_id(nova, neutron, server.id)
-            for vol_dict in vm.volume_ids:
-                if volume.id == vol_dict['id']:
-                    return vm
+    start_time = time.time()
+    while time.time() < start_time + timeout:
+        vm = get_server_object_by_id(nova, neutron, server.id)
+        for vol_dict in vm.volume_ids:
+            if volume.id == vol_dict['id']:
+                return vm
+        time.sleep(POLL_INTERVAL)
 
-        return None
-    else:
-        return get_server_object_by_id(nova, neutron, server.id)
+    raise NovaException(
+        'Attach failed on volume - {} and server - {}'.format(
+            volume.id, server.id))
 
 
-def detach_volume(nova, neutron, server, volume, timeout=None):
+def detach_volume(nova, neutron, server, volume, timeout=120):
     """
-    Attaches a volume to a server
+    Detaches a volume to a server. When the timeout parameter is used, a VmInst
+    object with the proper volume updates is returned unless it has not been
+    updated in the allotted amount of time then an Exception will be raised.
     :param nova: the nova client
     :param neutron: the neutron client
     :param server: the VMInst domain object
     :param volume: the Volume domain object
     :param timeout: denotes the amount of time to block to determine if the
-                    has been properly detached. When None, do not wait.
-    :return: the value from the nova call
+                    has been properly detached.
+    :return: updated VmInst object
     """
     nova.volumes.delete_server_volume(server.id, volume.id)
 
-    if timeout:
-        start_time = time.time()
-        while time.time() < start_time + timeout:
-            vm = get_server_object_by_id(nova, neutron, server.id)
-            found = False
+    start_time = time.time()
+    while time.time() < start_time + timeout:
+        vm = get_server_object_by_id(nova, neutron, server.id)
+        if len(vm.volume_ids) == 0:
+            return vm
+        else:
+            ids = list()
             for vol_dict in vm.volume_ids:
-                if volume.id == vol_dict['id']:
-                    found = True
-
-            if not found:
+                ids.append(vol_dict['id'])
+            if volume.id not in ids:
                 return vm
+        time.sleep(POLL_INTERVAL)
 
-        return None
-    else:
-        return get_server_object_by_id(nova, neutron, server.id)
+    raise NovaException(
+        'Detach failed on volume - {} server - {}'.format(
+            volume.id, server.id))
 
 
 class RebootType(enum.Enum):
diff --git a/snaps/openstack/utils/tests/nova_utils_tests.py b/snaps/openstack/utils/tests/nova_utils_tests.py
index 77dc5dd..9383088 100644
--- a/snaps/openstack/utils/tests/nova_utils_tests.py
+++ b/snaps/openstack/utils/tests/nova_utils_tests.py
@@ -33,6 +33,7 @@ from snaps.openstack.tests import openstack_tests
 from snaps.openstack.tests.os_source_file_test import OSComponentTestCase
 from snaps.openstack.utils import (
     nova_utils, neutron_utils, glance_utils, cinder_utils)
+from snaps.openstack.utils.nova_utils import NovaException
 
 __author__ = 'spisarski'
 
@@ -440,7 +441,8 @@ class NovaUtilsInstanceVolumeTests(OSComponentTestCase):
 
     def test_add_remove_volume(self):
         """
-        Tests the nova_utils.create_server() method
+        Tests the nova_utils.attach_volume() and detach_volume functions with
+        a timeout value
         :return:
         """
 
@@ -449,23 +451,27 @@ class NovaUtilsInstanceVolumeTests(OSComponentTestCase):
 
         # Attach volume to VM
         neutron = neutron_utils.neutron_client(self.os_creds)
-        nova_utils.attach_volume(
+        self.assertIsNotNone(nova_utils.attach_volume(
             self.nova, neutron, self.instance_creator.get_vm_inst(),
-            self.volume_creator.get_volume(), 120)
+            self.volume_creator.get_volume()))
 
-        vol_attach = cinder_utils.get_volume_by_id(
-            self.cinder, self.volume_creator.get_volume().id)
-        vm_attach = nova_utils.get_server_object_by_id(
-            self.nova, neutron, self.instance_creator.get_vm_inst().id)
+        vol_attach = None
+        attached = False
+        start_time = time.time()
+        while time.time() < start_time + 30:
+            vol_attach = cinder_utils.get_volume_by_id(
+                self.cinder, self.volume_creator.get_volume().id)
 
-        # Detach volume to VM
-        nova_utils.detach_volume(
-            self.nova, neutron, self.instance_creator.get_vm_inst(),
-            self.volume_creator.get_volume(), 120)
+            if len(vol_attach.attachments) > 0:
+                attached = True
+                break
 
-        vol_detach = cinder_utils.get_volume_by_id(
-            self.cinder, self.volume_creator.get_volume().id)
-        vm_detach = nova_utils.get_server_object_by_id(
+            time.sleep(3)
+
+        self.assertTrue(attached)
+        self.assertIsNotNone(vol_attach)
+
+        vm_attach = nova_utils.get_server_object_by_id(
             self.nova, neutron, self.instance_creator.get_vm_inst().id)
 
         # Validate Attachment
@@ -475,8 +481,84 @@ class NovaUtilsInstanceVolumeTests(OSComponentTestCase):
         self.assertEqual(vm_attach.volume_ids[0]['id'],
                          vol_attach.attachments[0]['volume_id'])
 
+        # Detach volume to VM
+        self.assertIsNotNone(nova_utils.detach_volume(
+            self.nova, neutron, self.instance_creator.get_vm_inst(),
+            self.volume_creator.get_volume()))
+
+        vol_detach = cinder_utils.get_volume_by_id(
+            self.cinder, self.volume_creator.get_volume().id)
+        vm_detach = nova_utils.get_server_object_by_id(
+            self.nova, neutron, self.instance_creator.get_vm_inst().id)
+
         # Validate Detachment
         self.assertIsNotNone(vol_detach)
         self.assertEqual(self.volume_creator.get_volume().id, vol_detach.id)
+
+        if len(vol_detach.attachments) > 0:
+            vol_detach = cinder_utils.get_volume_by_id(
+                self.cinder, self.volume_creator.get_volume().id)
+
         self.assertEqual(0, len(vol_detach.attachments))
         self.assertEqual(0, len(vm_detach.volume_ids))
+
+    def test_attach_volume_nowait(self):
+        """
+        Tests the nova_utils.attach_volume() with a timeout value that is too
+        small to have the volume attachment data to be included on the VmInst
+        object that was supposed to be returned
+        """
+
+        self.assertIsNotNone(self.volume_creator.get_volume())
+        self.assertEqual(0, len(self.volume_creator.get_volume().attachments))
+
+        # Attach volume to VM
+        neutron = neutron_utils.neutron_client(self.os_creds)
+        with self.assertRaises(NovaException):
+            nova_utils.attach_volume(
+                self.nova, neutron, self.instance_creator.get_vm_inst(),
+                self.volume_creator.get_volume(), 0)
+
+    def test_detach_volume_nowait(self):
+        """
+        Tests the nova_utils.detach_volume() with a timeout value that is too
+        small to have the volume attachment data to be included on the VmInst
+        object that was supposed to be returned
+        """
+
+        self.assertIsNotNone(self.volume_creator.get_volume())
+        self.assertEqual(0, len(self.volume_creator.get_volume().attachments))
+
+        # Attach volume to VM
+        neutron = neutron_utils.neutron_client(self.os_creds)
+        nova_utils.attach_volume(
+            self.nova, neutron, self.instance_creator.get_vm_inst(),
+            self.volume_creator.get_volume())
+
+        # Check VmInst for attachment
+        latest_vm = nova_utils.get_server_object_by_id(
+            self.nova, neutron, self.instance_creator.get_vm_inst().id)
+        self.assertEqual(1, len(latest_vm.volume_ids))
+
+        # Check Volume for attachment
+        vol_attach = None
+        attached = False
+        start_time = time.time()
+        while time.time() < start_time + 30:
+            vol_attach = cinder_utils.get_volume_by_id(
+                self.cinder, self.volume_creator.get_volume().id)
+
+            if len(vol_attach.attachments) > 0:
+                attached = True
+                break
+
+            time.sleep(3)
+
+        self.assertTrue(attached)
+        self.assertIsNotNone(vol_attach)
+
+        # Detach volume
+        with self.assertRaises(NovaException):
+            nova_utils.detach_volume(
+                self.nova, neutron, self.instance_creator.get_vm_inst(),
+                self.volume_creator.get_volume(), 0)