Don't check cinder volume states during attach
This patch changes how Nova interacts with volumes at attach time. Nova should rely upon Cinder's os-reserve to determine if the state of the volume is in a good/valid state for attaching. This fixes a race between when nova fetches the volume and calls os-reserve. This refactors the volume_api.check_attach a bit and adds a new check_availability_zone, which is still done on the Nova side. When Cinder's os-reserve supports passing in the availability zone, then this check can also be removed. This patch handles the volume attach API, which is not checked again in the compute manager. Future patches will handle other operations like boot from volume and swap volume. Partial-Bug: #1581230 Change-Id: I5b069ba3480257c061541fc6c19e044c31417b5e
This commit is contained in:
parent
3ab5b00fe2
commit
2e9959878b
@ -2987,7 +2987,8 @@ class API(base.Base):
|
||||
|
||||
def _check_attach_and_reserve_volume(self, context, volume_id, instance):
|
||||
volume = self.volume_api.get(context, volume_id)
|
||||
self.volume_api.check_attach(context, volume, instance=instance)
|
||||
self.volume_api.check_availability_zone(context, volume,
|
||||
instance=instance)
|
||||
self.volume_api.reserve_volume(context, volume_id)
|
||||
|
||||
def _attach_volume(self, context, instance, volume_id, device,
|
||||
|
@ -413,7 +413,8 @@ class ComputeCellsAPI(compute_api.API):
|
||||
disk_bus, device_type):
|
||||
"""Attach an existing volume to an existing instance."""
|
||||
volume = self.volume_api.get(context, volume_id)
|
||||
self.volume_api.check_attach(context, volume, instance=instance)
|
||||
self.volume_api.check_availability_zone(context, volume,
|
||||
instance=instance)
|
||||
|
||||
return self._call_to_cells(context, instance, 'attach_volume',
|
||||
volume_id, device, disk_bus, device_type)
|
||||
|
@ -9354,13 +9354,13 @@ class ComputeAPITestCase(BaseTestCase):
|
||||
|
||||
with test.nested(
|
||||
mock.patch.object(cinder.API, 'get', return_value=fake_volume),
|
||||
mock.patch.object(cinder.API, 'check_attach'),
|
||||
mock.patch.object(cinder.API, 'check_availability_zone'),
|
||||
mock.patch.object(cinder.API, 'reserve_volume'),
|
||||
mock.patch.object(compute_rpcapi.ComputeAPI,
|
||||
'reserve_block_device_name', return_value=bdm),
|
||||
mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume')
|
||||
) as (mock_get, mock_check_attach, mock_reserve_vol, mock_reserve_bdm,
|
||||
mock_attach):
|
||||
) as (mock_get, mock_check_availability_zone, mock_reserve_vol,
|
||||
mock_reserve_bdm, mock_attach):
|
||||
|
||||
self.compute_api.attach_volume(
|
||||
self.context, instance, 'fake-volume-id',
|
||||
@ -9371,7 +9371,7 @@ class ComputeAPITestCase(BaseTestCase):
|
||||
disk_bus='ide', device_type='cdrom')
|
||||
self.assertEqual(mock_get.call_args,
|
||||
mock.call(self.context, 'fake-volume-id'))
|
||||
self.assertEqual(mock_check_attach.call_args,
|
||||
self.assertEqual(mock_check_availability_zone.call_args,
|
||||
mock.call(
|
||||
self.context, fake_volume, instance=instance))
|
||||
mock_reserve_vol.assert_called_once_with(
|
||||
@ -9403,8 +9403,8 @@ class ComputeAPITestCase(BaseTestCase):
|
||||
|
||||
called = {}
|
||||
|
||||
def fake_check_attach(*args, **kwargs):
|
||||
called['fake_check_attach'] = True
|
||||
def fake_check_availability_zone(*args, **kwargs):
|
||||
called['fake_check_availability_zone'] = True
|
||||
|
||||
def fake_reserve_volume(*args, **kwargs):
|
||||
called['fake_reserve_volume'] = True
|
||||
@ -9424,7 +9424,8 @@ class ComputeAPITestCase(BaseTestCase):
|
||||
return bdm
|
||||
|
||||
self.stub_out('nova.volume.cinder.API.get', fake_volume_get)
|
||||
self.stub_out('nova.volume.cinder.API.check_attach', fake_check_attach)
|
||||
self.stub_out('nova.volume.cinder.API.check_availability_zone',
|
||||
fake_check_availability_zone)
|
||||
self.stub_out('nova.volume.cinder.API.reserve_volume',
|
||||
fake_reserve_volume)
|
||||
self.stub_out('nova.compute.rpcapi.ComputeAPI.'
|
||||
@ -9435,7 +9436,7 @@ class ComputeAPITestCase(BaseTestCase):
|
||||
|
||||
instance = self._create_fake_instance_obj()
|
||||
self.compute_api.attach_volume(self.context, instance, 1, device=None)
|
||||
self.assertTrue(called.get('fake_check_attach'))
|
||||
self.assertTrue(called.get('fake_check_availability_zone'))
|
||||
self.assertTrue(called.get('fake_reserve_volume'))
|
||||
self.assertTrue(called.get('fake_volume_get'))
|
||||
self.assertTrue(called.get('fake_rpc_reserve_block_device_name'))
|
||||
|
@ -378,9 +378,8 @@ class _ComputeAPIUnitTestMixIn(object):
|
||||
mock_v_api.get.return_value = volume
|
||||
self.compute_api.attach_volume(
|
||||
self.context, instance, volume['id'])
|
||||
mock_v_api.check_attach.assert_called_once_with(self.context,
|
||||
volume,
|
||||
instance=instance)
|
||||
mock_v_api.check_availability_zone.assert_called_once_with(
|
||||
self.context, volume, instance=instance)
|
||||
mock_v_api.reserve_volume.assert_called_once_with(self.context,
|
||||
volume['id'])
|
||||
mock_attach.assert_called_once_with(self.context,
|
||||
@ -405,9 +404,8 @@ class _ComputeAPIUnitTestMixIn(object):
|
||||
self.assertRaises(test.TestingException,
|
||||
self.compute_api.attach_volume,
|
||||
self.context, instance, volume['id'])
|
||||
mock_v_api.check_attach.assert_called_once_with(self.context,
|
||||
volume,
|
||||
instance=instance)
|
||||
mock_v_api.check_availability_zone.assert_called_once_with(
|
||||
self.context, volume, instance=instance)
|
||||
mock_v_api.reserve_volume.assert_called_once_with(self.context,
|
||||
volume['id'])
|
||||
self.assertEqual(0, mock_attach.call_count)
|
||||
@ -3898,9 +3896,8 @@ class ComputeAPIAPICellUnitTestCase(_ComputeAPIUnitTestMixIn,
|
||||
mock_v_api.get.return_value = volume
|
||||
self.compute_api.attach_volume(
|
||||
self.context, instance, volume['id'])
|
||||
mock_v_api.check_attach.assert_called_once_with(self.context,
|
||||
volume,
|
||||
instance=instance)
|
||||
mock_v_api.check_availability_zone.assert_called_once_with(
|
||||
self.context, volume, instance=instance)
|
||||
mock_attach.assert_called_once_with(self.context, instance,
|
||||
'attach_volume', volume['id'],
|
||||
None, None, None)
|
||||
|
@ -145,8 +145,7 @@ class CinderApiTestCase(test.NoDBTestCase):
|
||||
|
||||
@mock.patch.object(cinder.az, 'get_instance_availability_zone',
|
||||
return_value='zone1')
|
||||
def test_check_attach_availability_zone_differs(self,
|
||||
mock_get_instance_az):
|
||||
def test_check_availability_zone_differs(self, mock_get_instance_az):
|
||||
self.flags(cross_az_attach=False, group='cinder')
|
||||
volume = {'id': uuids.volume_id,
|
||||
'status': 'available',
|
||||
@ -155,7 +154,8 @@ class CinderApiTestCase(test.NoDBTestCase):
|
||||
instance = fake_instance_obj(self.ctx)
|
||||
|
||||
self.assertRaises(exception.InvalidVolume,
|
||||
self.api.check_attach, self.ctx, volume, instance)
|
||||
self.api.check_availability_zone,
|
||||
self.ctx, volume, instance)
|
||||
mock_get_instance_az.assert_called_once_with(self.ctx, instance)
|
||||
|
||||
def test_check_attach(self):
|
||||
|
@ -284,6 +284,14 @@ class API(object):
|
||||
if volume['attach_status'] == "attached":
|
||||
msg = _("volume %s already attached") % volume['id']
|
||||
raise exception.InvalidVolume(reason=msg)
|
||||
|
||||
self.check_availability_zone(context, volume, instance)
|
||||
|
||||
def check_availability_zone(self, context, volume, instance=None):
|
||||
"""Ensure that the availability zone is the same."""
|
||||
|
||||
# TODO(walter-boring): move this check to Cinder as part of
|
||||
# the reserve call.
|
||||
if instance and not CONF.cinder.cross_az_attach:
|
||||
instance_az = az.get_instance_availability_zone(context, instance)
|
||||
if instance_az != volume['availability_zone']:
|
||||
|
Loading…
Reference in New Issue
Block a user