diff --git a/heat/engine/resources/volume.py b/heat/engine/resources/volume.py index b4e829a4b3..89d27cd1bd 100644 --- a/heat/engine/resources/volume.py +++ b/heat/engine/resources/volume.py @@ -208,15 +208,24 @@ class VolumeAttachTask(object): """Return a co-routine which runs the task.""" logger.debug(str(self)) - va = self.clients.nova().volumes.create_server_volume( - server_id=self.server_id, - volume_id=self.volume_id, - device=self.device) - self.attachment_id = va.id + try: + vol = self.clients.cinder().volumes.get(self.volume_id) + vol.attach(self.server_id, self.device) + except clients.cinderclient.exceptions.NotFound: + raise exception.Error(_('%s - volume not found') % self.volume_id) + except clients.cinderclient.exceptions.BadRequest: + msg = _('Failed to attach %(vol)s to server %(srv)s') % { + 'vol': self.volume_id, 'srv': self.server_id} + raise exception.Error(msg) + + # NOTE(pshchelo): this relies on current behaviour of + # volume attachments in cinder: + # - only one attachment per volume + # - volume attachment id is the same as volume id it describes + self.attachment_id = vol.id yield - vol = self.clients.cinder().volumes.get(self.volume_id) - while vol.status == 'available' or vol.status == 'attaching': + while vol.status in ('available', 'attaching'): logger.debug(_('%(name)s - volume status: %(status)s') % { 'name': str(self), 'status': vol.status}) yield @@ -231,65 +240,43 @@ class VolumeAttachTask(object): class VolumeDetachTask(object): """A task for detaching a volume from a Nova server.""" - def __init__(self, stack, server_id, attachment_id): + def __init__(self, stack, server_id, volume_id): """ Initialise with the stack (for obtaining the clients), and the IDs of the server and volume. """ self.clients = stack.clients self.server_id = server_id - self.attachment_id = attachment_id + self.volume_id = volume_id def __str__(self): """Return a human-readable string description of the task.""" - return _('Removing attachment %(att)s from Instance %(srv)s') % { - 'att': self.attachment_id, 'srv': self.server_id} + return _('Removing volume %(vol)s from Instance %(srv)s') % { + 'vol': self.volume_id, 'srv': self.server_id} def __repr__(self): """Return a brief string description of the task.""" return '%s(%s -/> %s)' % (type(self).__name__, - self.attachment_id, + self.volume_id, self.server_id) def __call__(self): """Return a co-routine which runs the task.""" logger.debug(str(self)) - server_api = self.clients.nova().volumes - - # get reference to the volume while it is attached try: - nova_vol = server_api.get_server_volume(self.server_id, - self.attachment_id) - vol = self.clients.cinder().volumes.get(nova_vol.id) - except (clients.cinderclient.exceptions.NotFound, - clients.novaclient.exceptions.BadRequest, - clients.novaclient.exceptions.NotFound): - logger.warning(_('%s - volume not found') % str(self)) - return + vol = self.clients.cinder().volumes.get(self.volume_id) + attached_to = [att['server_id'] for att in vol.attachments] + if self.server_id not in attached_to: + msg = _('Volume %(vol)s is not attached to server %(srv)s') % { + 'vol': self.volume_id, 'srv': self.server_id} + raise exception.Error(msg) - # detach the volume using volume_attachment - try: - server_api.delete_server_volume(self.server_id, self.attachment_id) - except (clients.novaclient.exceptions.BadRequest, - clients.novaclient.exceptions.NotFound) as e: - logger.warning(_('%(res)s - %(err)s') % {'res': str(self), - 'err': e}) - - yield - - try: - vol.get() + vol.detach() + yield while vol.status in ('in-use', 'detaching'): logger.debug(_('%s - volume still in use') % str(self)) yield - - try: - server_api.delete_server_volume(self.server_id, - self.attachment_id) - except (clients.novaclient.exceptions.BadRequest, - clients.novaclient.exceptions.NotFound): - pass vol.get() logger.info(_('%(name)s - status: %(status)s') % { @@ -299,27 +286,31 @@ class VolumeDetachTask(object): except clients.cinderclient.exceptions.NotFound: logger.warning(_('%s - volume not found') % str(self)) + return + except clients.cinderclient.exceptions.BadRequest: + msg = _('Failed to detach %(vol)s from server %(srv)s') % { + 'vol': self.volume_id, 'srv': self.server_id} + raise exception.Error(msg) # The next check is needed for immediate reattachment when updating: - # as the volume info is taken from cinder, but the detach - # request is sent to nova, there might be some time - # between cinder marking volume as 'available' and - # nova removing attachment from it's own objects, so we + # there might be some time between cinder marking volume as 'available' + # and nova removing attachment from it's own objects, so we # check that nova already knows that the volume is detached - def server_has_attachment(server_id, attachment_id): + server_api = self.clients.nova().volumes + + def server_has_attachment(server_id, volume_id): try: - server_api.get_server_volume(server_id, attachment_id) + server_api.get_server_volume(server_id, volume_id) except clients.novaclient.exceptions.NotFound: return False return True - while server_has_attachment(self.server_id, self.attachment_id): - logger.info(_("Server %(srv)s still has attachment %(att)s.") % - {'att': self.attachment_id, 'srv': self.server_id}) + while server_has_attachment(self.server_id, self.volume_id): + logger.info(_("Server %(srv)s still has %(vol)s attached.") % + {'vol': self.volume_id, 'srv': self.server_id}) yield - logger.info(_("Volume %(vol)s is detached from server %(srv)s") % - {'vol': vol.id, 'srv': self.server_id}) + logger.info(_('%s - complete') % str(self)) class VolumeAttachment(resource.Resource): @@ -376,29 +367,23 @@ class VolumeAttachment(resource.Resource): def handle_delete(self): server_id = self.properties[self.INSTANCE_ID] - detach_task = VolumeDetachTask(self.stack, server_id, self.resource_id) + volume_id = self.properties[self.VOLUME_ID] + detach_task = VolumeDetachTask(self.stack, server_id, volume_id) scheduler.TaskRunner(detach_task)() def handle_update(self, json_snippet, tmpl_diff, prop_diff): checkers = [] if prop_diff: - # Even though some combinations of changed properties - # could be updated in UpdateReplace manner, - # we still first detach the old resource so that - # self.resource_id is not replaced prematurely volume_id = self.properties.get(self.VOLUME_ID) + server_id = self.properties.get(self.INSTANCE_ID) + detach_task = VolumeDetachTask(self.stack, server_id, volume_id) + checkers.append(scheduler.TaskRunner(detach_task)) + if self.VOLUME_ID in prop_diff: volume_id = prop_diff.get(self.VOLUME_ID) - device = self.properties.get(self.DEVICE) if self.DEVICE in prop_diff: device = prop_diff.get(self.DEVICE) - - server_id = self.properties.get(self.INSTANCE_ID) - detach_task = VolumeDetachTask(self.stack, server_id, - self.resource_id) - checkers.append(scheduler.TaskRunner(detach_task)) - if self.INSTANCE_ID in prop_diff: server_id = prop_diff.get(self.INSTANCE_ID) attach_task = VolumeAttachTask(self.stack, server_id, diff --git a/heat/tests/test_volume.py b/heat/tests/test_volume.py index d6c1d525b6..802ae20dc4 100644 --- a/heat/tests/test_volume.py +++ b/heat/tests/test_volume.py @@ -137,13 +137,16 @@ class VolumeTest(HeatTestCase): def _mock_create_server_volume_script(self, fva, server=u'WikiDatabase', volume='vol-123', - device=u'/dev/vdc', - update=False): - if not update: - clients.OpenStackClients.nova().MultipleTimes().AndReturn(self.fc) - self.fc.volumes.create_server_volume( - device=device, server_id=server, volume_id=volume).AndReturn(fva) + device=u'/dev/vdc'): self.cinder_fc.volumes.get(volume).AndReturn(fva) + self.m.StubOutWithMock(fva, 'attach') + fva.attach(server, device).AndReturn(None) + + def _mock_check_if_attached(self, fva, server=u'WikiDatabase'): + clients.OpenStackClients.nova().AndReturn(self.fc) + self.fc.volumes.get_server_volume(server, fva.id).AndReturn(fva) + self.fc.volumes.get_server_volume(server, fva.id).AndRaise( + clients.novaclient.exceptions.NotFound('NotFound')) def test_volume(self): fv = FakeVolume('creating', 'available') @@ -261,7 +264,7 @@ class VolumeTest(HeatTestCase): self.m.VerifyAll() - def test_volume_attachment_error(self): + def test_volume_attachment_volume_state_error(self): fv = FakeVolume('creating', 'available') fva = FakeVolume('attaching', 'error') stack_name = 'test_volume_attach_error_stack' @@ -286,7 +289,61 @@ class VolumeTest(HeatTestCase): self.m.VerifyAll() - def test_volume_attachment(self): + def test_volume_attachment_volume_not_found(self): + fv = FakeVolume('creating', 'available') + fva = FakeVolume('attaching', 'in-use') + stack_name = 'test_volume_attach_error_stack' + + self._mock_create_volume(fv, stack_name) + + self.cinder_fc.volumes.get(fva.id).AndRaise( + clients.cinderclient.exceptions.NotFound) + + self.m.ReplayAll() + + t = template_format.parse(volume_template) + t['Resources']['DataVolume']['Properties']['AvailabilityZone'] = 'nova' + stack = utils.parse_stack(t, stack_name=stack_name) + + scheduler.TaskRunner(stack['DataVolume'].create)() + self.assertEqual('available', fv.status) + rsrc = vol.VolumeAttachment('MountPoint', + t['Resources']['MountPoint'], + stack) + create = scheduler.TaskRunner(rsrc.create) + self.assertRaises(exception.ResourceFailure, create) + + self.m.VerifyAll() + + def test_volume_attachment_fail(self): + fv = FakeVolume('creating', 'available') + fva = FakeVolume('attaching', 'in-use') + stack_name = 'test_volume_attach_error_stack' + + self._mock_create_volume(fv, stack_name) + + self.cinder_fc.volumes.get(fva.id).AndReturn(fva) + self.m.StubOutWithMock(fva, 'attach') + fva.attach(u'WikiDatabase', '/dev/vdc').AndRaise( + clients.cinderclient.exceptions.BadRequest) + + self.m.ReplayAll() + + t = template_format.parse(volume_template) + t['Resources']['DataVolume']['Properties']['AvailabilityZone'] = 'nova' + stack = utils.parse_stack(t, stack_name=stack_name) + + scheduler.TaskRunner(stack['DataVolume'].create)() + self.assertEqual('available', fv.status) + rsrc = vol.VolumeAttachment('MountPoint', + t['Resources']['MountPoint'], + stack) + create = scheduler.TaskRunner(rsrc.create) + self.assertRaises(exception.ResourceFailure, create) + + self.m.VerifyAll() + + def test_volume_attachment_detachment(self): fv = FakeVolume('creating', 'available') fva = FakeVolume('attaching', 'in-use') stack_name = 'test_volume_attach_stack' @@ -297,17 +354,11 @@ class VolumeTest(HeatTestCase): # delete script fva = FakeVolume('in-use', 'available') - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) self.cinder_fc.volumes.get(fva.id).AndReturn(fva) - self.fc.volumes.delete_server_volume( - 'WikiDatabase', 'vol-123').MultipleTimes().AndReturn(None) - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) - self.fc.volumes.get_server_volume( - u'WikiDatabase', 'vol-123').AndRaise( - clients.novaclient.exceptions.NotFound('NotFound')) + self.m.StubOutWithMock(fva, 'detach') + fva.detach().AndReturn(None) + self._mock_check_if_attached(fva) self.m.ReplayAll() t = template_format.parse(volume_template) @@ -317,12 +368,39 @@ class VolumeTest(HeatTestCase): scheduler.TaskRunner(stack['DataVolume'].create)() self.assertEqual('available', fv.status) rsrc = self.create_attachment(t, stack, 'MountPoint') - + self.assertEqual(fva.id, rsrc.resource_id) scheduler.TaskRunner(rsrc.delete)() self.m.VerifyAll() - def test_volume_detachment_err(self): + def test_volume_detachment_not_attached(self): + fv = FakeVolume('creating', 'available') + fva = FakeVolume('attaching', 'in-use') + stack_name = 'test_volume_attach_stack' + + self._mock_create_volume(fv, stack_name) + + self._mock_create_server_volume_script(fva) + + # delete script + fva = FakeVolume('in-use', 'in-use', attached_to=u'OtherServer') + self.cinder_fc.volumes.get(fva.id).AndReturn(fva) + self.m.ReplayAll() + + t = template_format.parse(volume_template) + t['Resources']['DataVolume']['Properties']['AvailabilityZone'] = 'nova' + stack = utils.parse_stack(t, stack_name=stack_name) + + scheduler.TaskRunner(stack['DataVolume'].create)() + self.assertEqual('available', fv.status) + rsrc = self.create_attachment(t, stack, 'MountPoint') + self.assertEqual(fva.id, rsrc.resource_id) + delete = scheduler.TaskRunner(rsrc.delete) + self.assertRaises(exception.ResourceFailure, delete) + + self.m.VerifyAll() + + def test_volume_detachment_fail(self): fv = FakeVolume('creating', 'available') fva = FakeVolume('in-use', 'available') stack_name = 'test_volume_detach_stack' @@ -332,36 +410,11 @@ class VolumeTest(HeatTestCase): self._mock_create_server_volume_script(fva) # delete script - fva = FakeVolume('i-use', 'available') - self.m.StubOutWithMock(fva, 'get') - fva.get().MultipleTimes() - fva.status = "in-use" - - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) + fva = FakeVolume('in-use', 'in-use') self.cinder_fc.volumes.get(fva.id).AndReturn(fva) + self.m.StubOutWithMock(fva, 'detach') + fva.detach().AndRaise(clients.cinderclient.exceptions.BadRequest) - self.fc.volumes.delete_server_volume( - 'WikiDatabase', 'vol-123').AndRaise( - clients.novaclient.exceptions.BadRequest('Already detached')) - - self.fc.volumes.delete_server_volume( - 'WikiDatabase', 'vol-123').AndRaise( - clients.novaclient.exceptions.NotFound('Not found')) - - self.fc.volumes.delete_server_volume( - 'WikiDatabase', 'vol-123').AndRaise( - clients.novaclient.exceptions.NotFound('Not found')) - - self.fc.volumes.delete_server_volume( - 'WikiDatabase', 'vol-123').AndRaise( - clients.cinderclient.exceptions.NotFound('Not found')) - - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) - self.fc.volumes.get_server_volume( - u'WikiDatabase', 'vol-123').AndRaise( - clients.novaclient.exceptions.NotFound('NotFound')) self.m.ReplayAll() t = template_format.parse(volume_template) @@ -372,7 +425,8 @@ class VolumeTest(HeatTestCase): self.assertEqual('available', fv.status) rsrc = self.create_attachment(t, stack, 'MountPoint') - scheduler.TaskRunner(rsrc.delete)() + delete = scheduler.TaskRunner(rsrc.delete) + self.assertRaises(exception.ResourceFailure, delete) self.m.VerifyAll() @@ -386,8 +440,6 @@ class VolumeTest(HeatTestCase): self._mock_create_server_volume_script(fva) # delete script - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) self.cinder_fc.volumes.get(fva.id).AndRaise( clients.cinderclient.exceptions.NotFound('Not found')) @@ -416,16 +468,10 @@ class VolumeTest(HeatTestCase): # delete script volume_detach_cycle = 'in-use', 'detaching', 'available' fva = FakeLatencyVolume(life_cycle=volume_detach_cycle) - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) self.cinder_fc.volumes.get(fva.id).AndReturn(fva) - self.fc.volumes.delete_server_volume( - 'WikiDatabase', 'vol-123').MultipleTimes().AndReturn(None) - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) - self.fc.volumes.get_server_volume( - u'WikiDatabase', 'vol-123').AndRaise( - clients.novaclient.exceptions.NotFound('NotFound')) + self.m.StubOutWithMock(fva, 'detach') + fva.detach().AndReturn(None) + self._mock_check_if_attached(fva) self.m.ReplayAll() @@ -441,7 +487,7 @@ class VolumeTest(HeatTestCase): self.m.VerifyAll() - def test_volume_detach_with_error(self): + def test_volume_detach_volume_state_error(self): fv = FakeVolume('creating', 'available') fva = FakeVolume('attaching', 'in-use') stack_name = 'test_volume_attach_stack' @@ -452,11 +498,9 @@ class VolumeTest(HeatTestCase): # delete script fva = FakeVolume('in-use', 'error') - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) self.cinder_fc.volumes.get(fva.id).AndReturn(fva) - self.fc.volumes.delete_server_volume('WikiDatabase', - 'vol-123').AndReturn(None) + self.m.StubOutWithMock(fva, 'detach') + fva.detach().AndReturn(None) self.m.ReplayAll() t = template_format.parse(volume_template) @@ -506,20 +550,13 @@ class VolumeTest(HeatTestCase): # delete script fva = FakeVolume('in-use', 'available') - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) self.cinder_fc.volumes.get(fva.id).AndReturn(fva) - self.fc.volumes.delete_server_volume( - 'WikiDatabase', 'vol-123').MultipleTimes().AndReturn(None) - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) - self.fc.volumes.get_server_volume( - u'WikiDatabase', 'vol-123').AndRaise( - clients.novaclient.exceptions.NotFound('NotFound')) + self.m.StubOutWithMock(fva, 'detach') + fva.detach().AndReturn(None) + self._mock_check_if_attached(fva) # attach script - self._mock_create_server_volume_script(fva2, device=u'/dev/vdd', - update=True) + self._mock_create_server_volume_script(fva2, device=u'/dev/vdd') self.m.ReplayAll() @@ -564,25 +601,13 @@ class VolumeTest(HeatTestCase): # delete script fva = FakeVolume('in-use', 'available') - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) self.cinder_fc.volumes.get(fva.id).AndReturn(fva) - self.fc.volumes.delete_server_volume( - 'WikiDatabase', 'vol-123').MultipleTimes().AndReturn(None) - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) - self.fc.volumes.get_server_volume( - u'WikiDatabase', 'vol-123').AndRaise( - clients.novaclient.exceptions.NotFound('NotFound')) + self.m.StubOutWithMock(fva, 'detach') + fva.detach().AndReturn(None) + self._mock_check_if_attached(fva) # attach script - self._mock_create_server_volume_script(fv2a, volume='vol-456', - update=True) - #self.fc.volumes.create_server_volume( - #device=u'/dev/vdc', server_id=u'WikiDatabase', - #volume_id='vol-456').AndReturn(fv2a) - #self.cinder_fc.volumes.get('vol-456').AndReturn(fv2a) - + self._mock_create_server_volume_script(fv2a, volume='vol-456') self.m.ReplayAll() t = template_format.parse(volume_template) @@ -620,20 +645,13 @@ class VolumeTest(HeatTestCase): # delete script fva = FakeVolume('in-use', 'available') - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) self.cinder_fc.volumes.get(fva.id).AndReturn(fva) - self.fc.volumes.delete_server_volume( - 'WikiDatabase', 'vol-123').MultipleTimes().AndReturn(None) - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) - self.fc.volumes.get_server_volume( - u'WikiDatabase', 'vol-123').AndRaise( - clients.novaclient.exceptions.NotFound('NotFound')) + self.m.StubOutWithMock(fva, 'detach') + fva.detach().AndReturn(None) + self._mock_check_if_attached(fva) # attach script - self._mock_create_server_volume_script(fva2, server=u'WikiDatabase2', - update=True) + self._mock_create_server_volume_script(fva2, server=u'WikiDatabase2') self.m.ReplayAll() @@ -650,7 +668,6 @@ class VolumeTest(HeatTestCase): after = copy.deepcopy(t)['Resources']['MountPoint'] after['Properties']['VolumeId'] = 'vol-123' after['Properties']['InstanceId'] = 'WikiDatabase2' - #after['Properties']['Device'] = '/dev/vdd' scheduler.TaskRunner(rsrc.update, after)() self.assertEqual((rsrc.UPDATE, rsrc.COMPLETE), rsrc.state) @@ -988,16 +1005,10 @@ class VolumeTest(HeatTestCase): # delete script fva = FakeVolume('in-use', 'available') - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) self.cinder_fc.volumes.get(fva.id).AndReturn(fva) - self.fc.volumes.delete_server_volume( - 'WikiDatabase', 'vol-123').MultipleTimes().AndReturn(None) - self.fc.volumes.get_server_volume(u'WikiDatabase', - 'vol-123').AndReturn(fva) - self.fc.volumes.get_server_volume( - u'WikiDatabase', 'vol-123').AndRaise( - clients.novaclient.exceptions.NotFound('NotFound')) + self.m.StubOutWithMock(fva, 'detach') + fva.detach().AndReturn(None) + self._mock_check_if_attached(fva) self.m.ReplayAll() @@ -1028,9 +1039,11 @@ class FakeVolume(object): status = 'attaching' id = 'vol-123' - def __init__(self, initial_status, final_status, **attrs): + def __init__(self, initial_status, final_status, + attached_to=u'WikiDatabase', **attrs): self.status = initial_status self.final_status = final_status + self.attachments = [{'server_id': attached_to}] for key, value in attrs.iteritems(): setattr(self, key, value) @@ -1043,18 +1056,26 @@ class FakeVolume(object): def delete(self): pass + def attach(self, server_id, mount): + pass + + def detach(self): + pass + class FakeLatencyVolume(object): status = 'attaching' id = 'vol-123' - def __init__(self, life_cycle=('creating', 'available'), **attrs): + def __init__(self, life_cycle=('creating', 'available'), + attached_to=u'WikiDatabase', **attrs): if not isinstance(life_cycle, tuple): raise exception.Error('life_cycle need to be a tuple.') if not len(life_cycle): raise exception.Error('life_cycle should not be an empty tuple.') self.life_cycle = iter(life_cycle) self.status = next(self.life_cycle) + self.attachments = [{'server_id': attached_to}] for key, value in attrs.iteritems(): setattr(self, key, value) @@ -1064,6 +1085,9 @@ class FakeLatencyVolume(object): def update(self, **kw): pass + def detach(self): + pass + class FakeBackup(FakeVolume): status = 'creating'