openstack
/
nova
Code Issues Proposed changes

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:
Walter A. Boring IV 2016-05-12 14:59:29 -07:00 committed by Matt Riedemann
parent 3ab5b00fe2
commit 2e9959878b
6 changed files with 30 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
nova/compute/api.py
View File

@ -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,

3
nova/compute/cells_api.py
View File

@ -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)

17
nova/tests/unit/compute/test_compute.py
View File

@ -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'))

15
nova/tests/unit/compute/test_compute_api.py
View File

@ -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)

6
nova/tests/unit/volume/test_cinder.py
View File

@ -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):

8
nova/volume/cinder.py
View File

@ -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']: