From 230d07e9002371bdb0030c9199df35fc6360a0a2 Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Thu, 24 Mar 2011 03:26:32 -0700 Subject: Test for attach / detach (and associated fixes) --- nova/api/openstack/__init__.py | 2 +- nova/api/openstack/volume_attachments.py | 77 ++++++---- nova/tests/integrated/api/client.py | 15 ++ nova/tests/integrated/integrated_helpers.py | 49 +++++++ nova/tests/integrated/test_login.py | 12 +- nova/tests/integrated/test_servers.py | 37 +---- nova/tests/integrated/test_volumes.py | 215 +++++++++++++++++++++++----- nova/volume/driver.py | 12 +- 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): @@ -667,6 +669,10 @@ class LoggingVolumeDriver(VolumeDriver): _LOGS = [] + @staticmethod + def clear_logs(): + LoggingVolumeDriver._LOGS = [] + @staticmethod def log_action(action, parameters): """Logs the command.""" -- cgit