From 2e9959878bd7f2cc1f42c2b823aaff0163ef398c Mon Sep 17 00:00:00 2001 From: "Walter A. Boring IV" Date: Thu, 12 May 2016 14:59:29 -0700 Subject: [PATCH] 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 --- nova/compute/api.py | 3 ++- nova/compute/cells_api.py | 3 ++- nova/tests/unit/compute/test_compute.py | 17 +++++++++-------- nova/tests/unit/compute/test_compute_api.py | 15 ++++++--------- nova/tests/unit/volume/test_cinder.py | 6 +++--- nova/volume/cinder.py | 8 ++++++++ 6 files changed, 30 insertions(+), 22 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 85b93323989a..10a480cf5093 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -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, diff --git a/nova/compute/cells_api.py b/nova/compute/cells_api.py index 3743c8e7dc6b..16539304ffb8 100644 --- a/nova/compute/cells_api.py +++ b/nova/compute/cells_api.py @@ -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) diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 1e8350f5d5c9..7176101b9058 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -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')) diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index d9436bd5f910..c9ba95c78844 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -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) diff --git a/nova/tests/unit/volume/test_cinder.py b/nova/tests/unit/volume/test_cinder.py index 588935c08a1d..762b551bb933 100644 --- a/nova/tests/unit/volume/test_cinder.py +++ b/nova/tests/unit/volume/test_cinder.py @@ -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): diff --git a/nova/volume/cinder.py b/nova/volume/cinder.py index 6ac21bc0ae68..6be828a63bad 100644 --- a/nova/volume/cinder.py +++ b/nova/volume/cinder.py @@ -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']: