Test for attach / detach (and associated fixes)

8 files changed, 312 insertions, 107 deletions

diff --git a/nova/api/openstack/__init__.py b/nova/api/openstack/__init__.py
index 54d8a738d..e8aa4821b 100644
--- a/nova/api/openstack/__init__.py
+++ b/nova/api/openstack/__init__.py
@@ -134,7 +134,7 @@ class APIRouter(wsgi.Router):
                         controller=volumes.Controller(),
                         collection={'detail': 'GET'})
 
-        mapper.resource("volume_attachment", "volume_attachment",
+        mapper.resource("volume_attachment", "volume_attachments",
                         controller=volume_attachments.Controller(),
                         parent_resource=dict(member_name='server',
                         collection_name='servers'))
diff --git a/nova/api/openstack/volume_attachments.py b/nova/api/openstack/volume_attachments.py
index 1cb2c9494..2ce681e19 100644
--- a/nova/api/openstack/volume_attachments.py
+++ b/nova/api/openstack/volume_attachments.py
@@ -35,27 +35,29 @@ FLAGS = flags.FLAGS
 def _translate_detail_view(context, volume):
     """ Maps keys for details view"""
 
-    v = _translate_summary_view(context, volume)
+    d = _translate_summary_view(context, volume)
 
     # No additional data / lookups at the moment
 
-    return v
+    return d
 
 
-def _translate_summary_view(context, volume):
+def _translate_summary_view(context, vol):
     """ Maps keys for summary view"""
-    v = {}
+    d = {}
+
+    volume_id = vol['id']
 
-    volume_id = volume['id']
-    
     # NOTE(justinsb): We use the volume id as the id of the attachment object
-    v['id'] = volume_id
-    
-    v['volumeId'] = volume_id
-    v['serverId'] = volume['instance_id']
-    v['device'] = volume['mountpoint']
+    d['id'] = volume_id
 
-    return v
+    d['volumeId'] = volume_id
+    if vol.get('instance_id'):
+        d['serverId'] = vol['instance_id']
+    if vol.get('mountpoint'):
+        d['device'] = vol['mountpoint']
+
+    return d
 
 
 class Controller(wsgi.Controller):
@@ -82,16 +84,22 @@ class Controller(wsgi.Controller):
         return self._items(req, server_id,
                            entity_maker=_translate_summary_view)
 
-    def show(self, req, id):
+    def show(self, req, server_id, id):
         """Return data about the given volume"""
         context = req.environ['nova.context']
 
+        volume_id = id
         try:
-            vol = self.volume_api.get(context, id)
+            vol = self.volume_api.get(context, volume_id)
         except exception.NotFound:
+            LOG.debug("volume_id not found")
             return faults.Fault(exc.HTTPNotFound())
 
-        return {'volume': _translate_detail_view(context, vol)}
+        if str(vol['instance_id']) != server_id:
+            LOG.debug("instance_id != server_id")
+            return faults.Fault(exc.HTTPNotFound())
+
+        return {'volumeAttachment': _translate_detail_view(context, vol)}
 
     def create(self, req, server_id):
         """ Attach a volume to an instance """
@@ -109,15 +117,29 @@ class Controller(wsgi.Controller):
                 " at %(device)s") % locals()
         LOG.audit(msg, context=context)
 
-        self.compute_api.attach_volume(context,
-                                       instance_id=instance_id,
-                                       volume_id=volume_id,
-                                       device=device)
-        vol = self.volume_api.get(context, volume_id)
+        try:
+            self.compute_api.attach_volume(context,
+                                           instance_id=instance_id,
+                                           volume_id=volume_id,
+                                           device=device)
+        except exception.NotFound:
+            return faults.Fault(exc.HTTPNotFound())
+
+        # The attach is async
+        attachment = {}
+        attachment['id'] = volume_id
+        attachment['volumeId'] = volume_id
 
-        retval = _translate_detail_view(context, vol)
+        # NOTE(justinsb): And now, we have a problem...
+        # The attach is async, so there's a window in which we don't see
+        #  the attachment (until the attachment completes).  We could also
+        #  get problems with concurrent requests.  I think we need an
+        #  attachment state, and to write to the DB here, but that's a bigger
+        #  change.
+        # For now, we'll probably have to rely on libraries being smart
 
-        return {'volumeAttachment': retval}
+        # TODO: How do I return "accepted" here??
+        return {'volumeAttachment': attachment}
 
     def update(self, _req, _server_id, _id):
         """ Update a volume attachment.  We don't currently support this."""
@@ -130,10 +152,15 @@ class Controller(wsgi.Controller):
         volume_id = id
         LOG.audit(_("Detach volume %s"), volume_id, context=context)
 
-        vol = self.volume_api.get(context, volume_id)
-        if vol['instance_id'] != server_id:
+        try:
+            vol = self.volume_api.get(context, volume_id)
+        except exception.NotFound:
+            return faults.Fault(exc.HTTPNotFound())
+
+        if str(vol['instance_id']) != server_id:
+            LOG.debug("instance_id != server_id")
             return faults.Fault(exc.HTTPNotFound())
-    
+
         self.compute_api.detach_volume(context,
                                        volume_id=volume_id)
 
diff --git a/nova/tests/integrated/api/client.py b/nova/tests/integrated/api/client.py
index 7a4c3198e..deb7fd981 100644
--- a/nova/tests/integrated/api/client.py
+++ b/nova/tests/integrated/api/client.py
@@ -223,3 +223,18 @@ class TestOpenStackClient(object):
     def delete_volume(self, volume_id):
         return self.api_delete('/volumes/%s' % volume_id)
 
+    def get_server_volume(self, server_id, attachment_id):
+        return self.api_get('/servers/%s/volume_attachments/%s' %
+                            (server_id, attachment_id))['volumeAttachment']
+    
+    def get_server_volumes(self, server_id):
+        return self.api_get('/servers/%s/volume_attachments' %
+                            (server_id))['volumeAttachments']
+
+    def post_server_volume(self, server_id, volume_attachment):
+        return self.api_post('/servers/%s/volume_attachments' %
+                        (server_id), volume_attachment)['volumeAttachment']
+
+    def delete_server_volume(self, server_id, attachment_id):
+        return self.api_delete('/servers/%s/volume_attachments/%s' %
+                            (server_id, attachment_id))
diff --git a/nova/tests/integrated/integrated_helpers.py b/nova/tests/integrated/integrated_helpers.py
index f24759032..b520cc5d3 100644
--- a/nova/tests/integrated/integrated_helpers.py
+++ b/nova/tests/integrated/integrated_helpers.py
@@ -125,6 +125,7 @@ class IntegratedUnitTestContext(object):
         self._configure_project(self.project_name, self.test_user)
 
     def _start_services(self):
+        self._start_compute_service()
         self._start_volume_service()
         self._start_scheduler_service()
 
@@ -133,6 +134,12 @@ class IntegratedUnitTestContext(object):
         if not self.api_service:
             self._start_api_service()
 
+    def _start_compute_service(self):
+        compute_service = service.Service.create(binary='nova-compute')
+        compute_service.start()
+        self.services.append(compute_service)
+        return compute_service
+
     def _start_volume_service(self):
         volume_service = service.Service.create(binary='nova-volume')
         volume_service.start()
@@ -223,3 +230,45 @@ class IntegratedUnitTestContext(object):
         # WSGI shutdown broken :-(
         # bug731668
         #IntegratedUnitTestContext.__INSTANCE = None
+
+
+class _IntegratedTestBase(test.TestCase):
+    def setUp(self):
+        super(_IntegratedTestBase, self).setUp()
+
+        self._setup_flags()
+
+        context = IntegratedUnitTestContext.startup()
+        self.user = context.test_user
+        self.api = self.user.openstack_api
+
+    def tearDown(self):
+        IntegratedUnitTestContext.shutdown()
+        super(_IntegratedTestBase, self).tearDown()
+
+    def _setup_flags(self):
+        """An opportunity to setup flags, before the services are started"""
+        pass
+
+    def _build_minimal_create_server_request(self):
+        server = {}
+
+        image = self.user.get_valid_image(create=True)
+        image_id = image['id']
+
+        #TODO(justinsb): This is FUBAR
+        image_id = abs(hash(image_id))
+
+        # We now have a valid imageId
+        server['imageId'] = image_id
+
+        # Set a valid flavorId
+        flavor = self.api.get_flavors()[0]
+        LOG.debug("Using flavor: %s" % flavor)
+        server['flavorId'] = flavor['id']
+
+        # Set a valid server name
+        server_name = self.user.get_unused_server_name()
+        server['name'] = server_name
+
+        return server
diff --git a/nova/tests/integrated/test_login.py b/nova/tests/integrated/test_login.py
index 501f8c919..cc3d555d0 100644
--- a/nova/tests/integrated/test_login.py
+++ b/nova/tests/integrated/test_login.py
@@ -30,17 +30,7 @@ FLAGS = flags.FLAGS
 FLAGS.verbose = True
 
 
-class LoginTest(test.TestCase):
-    def setUp(self):
-        super(LoginTest, self).setUp()
-        context = integrated_helpers.IntegratedUnitTestContext.startup()
-        self.user = context.test_user
-        self.api = self.user.openstack_api
-
-    def tearDown(self):
-        integrated_helpers.IntegratedUnitTestContext.shutdown()
-        super(LoginTest, self).tearDown()
-
+class LoginTest(integrated_helpers._IntegratedTestBase):
     def test_login(self):
         """Simple check - we list flavors - so we know we're logged in"""
         flavors = self.api.get_flavors()
diff --git a/nova/tests/integrated/test_servers.py b/nova/tests/integrated/test_servers.py
index 3c38295c5..2b5d3324a 100644
--- a/nova/tests/integrated/test_servers.py
+++ b/nova/tests/integrated/test_servers.py
@@ -31,20 +31,10 @@ FLAGS = flags.FLAGS
 FLAGS.verbose = True
 
 
-class ServersTest(test.TestCase):
-    def setUp(self):
-        super(ServersTest, self).setUp()
-
+class ServersTest(integrated_helpers._IntegratedTestBase):
+    def _setup_flags(self):
         self.flags(image_service='nova.image.fake.MockImageService')
 
-        context = integrated_helpers.IntegratedUnitTestContext.startup()
-        self.user = context.test_user
-        self.api = self.user.openstack_api
-
-    def tearDown(self):
-        integrated_helpers.IntegratedUnitTestContext.shutdown()
-        super(ServersTest, self).tearDown()
-
     def test_get_servers(self):
         """Simple check that listing servers works."""
         servers = self.api.get_servers()
@@ -145,29 +135,6 @@ class ServersTest(test.TestCase):
         # Should be gone
         self.assertFalse(found_server)
 
-    def _build_minimal_create_server_request(self):
-        server = {}
-
-        image = self.user.get_valid_image(create=True)
-        image_id = image['id']
-
-        #TODO(justinsb): This is FUBAR
-        image_id = abs(hash(image_id))
-
-        # We now have a valid imageId
-        server['imageId'] = image_id
-
-        # Set a valid flavorId
-        flavor = self.api.get_flavors()[0]
-        LOG.debug("Using flavor: %s" % flavor)
-        server['flavorId'] = flavor['id']
-
-        # Set a valid server name
-        server_name = self.user.get_unused_server_name()
-        server['name'] = server_name
-
-        return server
-
 # TODO(justinsb): Enable this unit test when the metadata bug is fixed
 #    def test_create_server_with_metadata(self):
 #        """Creates a server with metadata"""
diff --git a/nova/tests/integrated/test_volumes.py b/nova/tests/integrated/test_volumes.py
index 66b773db2..f69361fb0 100644
--- a/nova/tests/integrated/test_volumes.py
+++ b/nova/tests/integrated/test_volumes.py
@@ -32,20 +32,15 @@ FLAGS = flags.FLAGS
 FLAGS.verbose = True
 
 
-class VolumesTest(test.TestCase):
+class VolumesTest(integrated_helpers._IntegratedTestBase):
     def setUp(self):
         super(VolumesTest, self).setUp()
+        driver.LoggingVolumeDriver.clear_logs()
 
+    def _setup_flags(self):
         self.flags(image_service='nova.image.fake.MockImageService',
                    volume_driver='nova.volume.driver.LoggingVolumeDriver')
-
-        context = integrated_helpers.IntegratedUnitTestContext.startup()
-        self.user = context.test_user
-        self.api = self.user.openstack_api
-
-    def tearDown(self):
-        integrated_helpers.IntegratedUnitTestContext.shutdown()
-        super(VolumesTest, self).tearDown()
+        self.flags(use_local_volumes=False)  # Avoids calling local_path
 
     def test_get_volumes(self):
         """Simple check that listing volumes works"""
@@ -53,10 +48,34 @@ class VolumesTest(test.TestCase):
         for volume in volumes:
             LOG.debug("volume: %s" % volume)
 
+    def _poll_while(self, volume_id, continue_states, max_retries=5):
+        """ Poll (briefly) while the state is in continue_states"""
+        retries = 0
+        while True:
+            try:
+                found_volume = self.api.get_volume(volume_id)
+            except client.OpenStackApiNotFoundException:
+                found_volume = None
+                LOG.debug("Got 404, proceeding")
+                break
+
+            LOG.debug("Found %s" % found_volume)
+
+            self.assertEqual(volume_id, found_volume['id'])
+
+            if not found_volume['status'] in continue_states:
+                break
+
+            time.sleep(1)
+            retries = retries + 1
+            if retries > max_retries:
+                break
+        return found_volume
+
     def test_create_and_delete_volume(self):
         """Creates and deletes a volume"""
 
-        # Create volume with name
+        # Create volume
         created_volume = self.api.post_volume({'volume': {'size': 1}})
         LOG.debug("created_volume: %s" % created_volume)
         self.assertTrue(created_volume['id'])
@@ -72,14 +91,7 @@ class VolumesTest(test.TestCase):
         self.assertTrue(created_volume_id in volume_names)
 
         # Wait (briefly) for creation. Delay is due to the 'message queue'
-        retries = 0
-        while found_volume['status'] == 'creating':
-            LOG.debug("Found %s" % found_volume)
-            time.sleep(1)
-            found_volume = self.api.get_volume(created_volume_id)
-            retries = retries + 1
-            if retries > 5:
-                break
+        found_volume = self._poll_while(created_volume_id, ['creating'])
 
         # It should be available...
         self.assertEqual('available', found_volume['status'])
@@ -88,18 +100,7 @@ class VolumesTest(test.TestCase):
         self.api.delete_volume(created_volume_id)
 
         # Wait (briefly) for deletion. Delay is due to the 'message queue'
-        for retries in range(5):
-            try:
-                found_volume = self.api.get_volume(created_volume_id)
-            except client.OpenStackApiNotFoundException:
-                found_volume = None
-                LOG.debug("Got 404, proceeding")
-                break
-
-            LOG.debug("Found_volume=%s" % found_volume)
-            if found_volume['status'] != 'deleting':
-                break
-            time.sleep(1)
+        found_volume = self._poll_while(created_volume_id, ['deleting'])
 
         # Should be gone
         self.assertFalse(found_volume)
@@ -110,7 +111,7 @@ class VolumesTest(test.TestCase):
                             'create_volume',
                             id=created_volume_id)
         LOG.debug("Create_Actions: %s" % create_actions)
-        
+
         self.assertEquals(1, len(create_actions))
         create_action = create_actions[0]
         self.assertEquals(create_action['id'], created_volume_id)
@@ -132,6 +133,156 @@ class VolumesTest(test.TestCase):
         delete_action = export_actions[0]
         self.assertEquals(delete_action['id'], created_volume_id)
 
+    def test_attach_and_detach_volume(self):
+        """Creates, attaches, detaches and deletes a volume"""
+
+        # Create server
+        server_req = {'server': self._build_minimal_create_server_request()}
+        # NOTE(justinsb): Create an extra server so that server_id != volume_id
+        self.api.post_server(server_req)
+        created_server = self.api.post_server(server_req)
+        LOG.debug("created_server: %s" % created_server)
+        server_id = created_server['id']
+
+        # Create volume
+        created_volume = self.api.post_volume({'volume': {'size': 1}})
+        LOG.debug("created_volume: %s" % created_volume)
+        volume_id = created_volume['id']
+        self._poll_while(volume_id, ['creating'])
+
+        # Check we've got different IDs
+        self.assertNotEqual(server_id, volume_id)
+
+        # List current server attachments - should be none
+        attachments = self.api.get_server_volumes(server_id)
+        self.assertEquals([], attachments)
+
+        # Template attach request
+        device = '/dev/sdc'
+        attach_req = { 'device': device }
+        post_req = { 'volumeAttachment': attach_req }
+
+        # Try to attach to a non-existent volume; should fail
+        attach_req['volumeId'] = 3405691582
+        self.assertRaises(client.OpenStackApiNotFoundException,
+                          self.api.post_server_volume, server_id, post_req)
+
+        # Try to attach to a non-existent server; should fail
+        attach_req['volumeId'] = volume_id
+        self.assertRaises(client.OpenStackApiNotFoundException,
+                          self.api.post_server_volume, 3405691582, post_req)
+
+        # Should still be no attachments...
+        attachments = self.api.get_server_volumes(server_id)
+        self.assertEquals([], attachments)
+
+        # Do a real attach
+        attach_req['volumeId'] = volume_id
+        attach_result = self.api.post_server_volume(server_id, post_req)
+        LOG.debug(_("Attachment = %s") % attach_result)
+
+        attachment_id = attach_result['id']
+        self.assertEquals(volume_id, attach_result['volumeId'])
+
+        # These fields aren't set because it's async
+        #self.assertEquals(server_id, attach_result['serverId'])
+        #self.assertEquals(device, attach_result['device'])
+
+        # This is just an implementation detail, but let's check it...
+        self.assertEquals(volume_id, attachment_id)
+
+        # NOTE(justinsb): There's an issue with the attach code, in that
+        #  it's currently asynchronous and not recorded until the attach
+        #  completes.  So the caller must be 'smart', like this...
+        attach_done = None
+        retries = 0
+        while True:
+            try:
+                attach_done = self.api.get_server_volume(server_id,
+                                                             attachment_id)
+                break
+            except client.OpenStackApiNotFoundException:
+                LOG.debug("Got 404, waiting")
+
+            time.sleep(1)
+            retries = retries + 1
+            if retries > 10:
+                break
+
+        expect_attach = {}
+        expect_attach['id'] = volume_id
+        expect_attach['volumeId'] = volume_id
+        expect_attach['serverId'] = server_id
+        expect_attach['device'] = device
+
+        self.assertEqual(expect_attach, attach_done)
+
+        # Should be one attachemnt
+        attachments = self.api.get_server_volumes(server_id)
+        self.assertEquals([expect_attach], attachments)
+
+        # Should be able to get details
+        attachment_info = self.api.get_server_volume(server_id, attachment_id)
+        self.assertEquals(expect_attach, attachment_info)
+
+        # Getting details on a different id should fail
+        self.assertRaises(client.OpenStackApiNotFoundException,
+                          self.api.get_server_volume, server_id, 3405691582)
+        self.assertRaises(client.OpenStackApiNotFoundException,
+                          self.api.get_server_volume,
+                          3405691582, attachment_id)
+
+        # Trying to detach a different id should fail
+        self.assertRaises(client.OpenStackApiNotFoundException,
+                          self.api.delete_server_volume, server_id, 3405691582)
+
+        # Detach should work
+        self.api.delete_server_volume(server_id, attachment_id)
+
+        # Again, it's async, so wait...
+        retries = 0
+        while True:
+            try:
+                attachment = self.api.get_server_volume(server_id,
+                                                        attachment_id)
+                LOG.debug("Attachment still there: %s" % attachment)
+            except client.OpenStackApiNotFoundException:
+                LOG.debug("Got 404, delete done")
+                break
+
+            time.sleep(1)
+            retries = retries + 1
+            self.assertTrue(retries < 10)
+
+        # Should be no attachments again
+        attachments = self.api.get_server_volumes(server_id)
+        self.assertEquals([], attachments)
+
+        LOG.debug("Logs: %s" % driver.LoggingVolumeDriver.all_logs())
+
+        # Discover_volume and undiscover_volume are called from compute
+        #  on attach/detach
+
+        disco_moves = driver.LoggingVolumeDriver.logs_like(
+                            'discover_volume',
+                            id=volume_id)
+        LOG.debug("discover_volume actions: %s" % disco_moves)
+
+        self.assertEquals(1, len(disco_moves))
+        disco_move = disco_moves[0]
+        self.assertEquals(disco_move['id'], volume_id)
+
+        last_days_of_disco_moves = driver.LoggingVolumeDriver.logs_like(
+                            'undiscover_volume',
+                            id=volume_id)
+        LOG.debug("undiscover_volume actions: %s" % last_days_of_disco_moves)
+
+        self.assertEquals(1, len(last_days_of_disco_moves))
+        undisco_move = last_days_of_disco_moves[0]
+        self.assertEquals(undisco_move['id'], volume_id)
+        self.assertEquals(undisco_move['mountpoint'], device)
+        self.assertEquals(undisco_move['instance_id'], server_id)
+
 
 if __name__ == "__main__":
-    unittest.main()
\ No newline at end of file
+    unittest.main()
diff --git a/nova/volume/driver.py b/nova/volume/driver.py
index 148e5facd..045974fa3 100644
--- a/nova/volume/driver.py
+++ b/nova/volume/driver.py
@@ -135,7 +135,7 @@ class VolumeDriver(object):
         """Removes an export for a logical volume."""
         raise NotImplementedError()
 
-    def discover_volume(self, volume):
+    def discover_volume(self, context, volume):
         """Discover volume on a remote host."""
         raise NotImplementedError()
 
@@ -574,6 +574,8 @@ class RBDDriver(VolumeDriver):
 
     def discover_volume(self, volume):
         """Discover volume on a remote host"""
+        #NOTE(justinsb): This is messed up... discover_volume takes 3 args
+        # but then that would break local_path
         return "rbd:%s/%s" % (FLAGS.rbd_pool, volume['name'])
 
     def undiscover_volume(self, volume):
@@ -622,7 +624,7 @@ class SheepdogDriver(VolumeDriver):
         """Removes an export for a logical volume"""
         pass
 
-    def discover_volume(self, volume):
+    def discover_volume(self, context, volume):
         """Discover volume on a remote host"""
         return "sheepdog:%s" % volume['name']
 
@@ -656,7 +658,7 @@ class LoggingVolumeDriver(VolumeDriver):
     def remove_export(self, context, volume):
         self.log_action('remove_export', volume)
 
-    def discover_volume(self, volume):
+    def discover_volume(self, context, volume):
         self.log_action('discover_volume', volume)
 
     def undiscover_volume(self, volume):
@@ -668,6 +670,10 @@ class LoggingVolumeDriver(VolumeDriver):
     _LOGS = []
 
     @staticmethod
+    def clear_logs():
+        LoggingVolumeDriver._LOGS = []
+
+    @staticmethod
     def log_action(action, parameters):
         """Logs the command."""
         LOG.debug(_("LoggingVolumeDriver: %s") % (action))