From 1cea4f013523ca66be5115687a7c129869900d60 Mon Sep 17 00:00:00 2001 From: Kevin_Zheng Date: Thu, 2 Nov 2017 16:13:17 +0800 Subject: [PATCH] Add instance action record for attach/detach/swap volumes We currently don't record volume attach/detach/swap instance actions. This is useful for auditing and debugging. This patch adds volume attach/detach/swap actions. Change-Id: I0a3d15f3e3d0d8d920a79b519e17e3228e99f293 partial-implements: blueprint fill-the-gap-for-instance-action-records --- nova/compute/api.py | 31 ++++++++++--- nova/compute/instance_actions.py | 3 ++ nova/compute/manager.py | 3 ++ nova/tests/unit/compute/test_compute.py | 45 +++++++++++++++---- nova/tests/unit/compute/test_compute_api.py | 17 +++++-- nova/tests/unit/compute/test_compute_mgr.py | 5 +++ ...e-action-record-gaps-14b36eba313d6d87.yaml | 3 ++ 7 files changed, 91 insertions(+), 16 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index b9b24b9919c2..6838409d3786 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -48,6 +48,7 @@ from nova.compute import power_state from nova.compute import rpcapi as compute_rpcapi from nova.compute import task_states from nova.compute import utils as compute_utils +from nova.compute.utils import wrap_instance_event from nova.compute import vm_states from nova import conductor import nova.conf @@ -3558,6 +3559,8 @@ class API(base.Base): device_type=device_type, tag=tag) try: self._check_attach_and_reserve_volume(context, volume_id, instance) + self._record_action_start( + context, instance, instance_actions.ATTACH_VOLUME) self.compute_rpcapi.attach_volume(context, instance, volume_bdm) except Exception: with excutils.save_and_reraise_exception(): @@ -3577,16 +3580,22 @@ class API(base.Base): therefore the actual attachment will be performed once the instance will be unshelved. """ + @wrap_instance_event(prefix='api') + def attach_volume(self, context, v_id, instance, dev): + self.volume_api.attach(context, + v_id, + instance.uuid, + dev) volume_bdm = self._create_volume_bdm( context, instance, device, volume_id, disk_bus=disk_bus, device_type=device_type, is_local_creation=True) try: self._check_attach_and_reserve_volume(context, volume_id, instance) - self.volume_api.attach(context, - volume_id, - instance.uuid, - device) + self._record_action_start( + context, instance, + instance_actions.ATTACH_VOLUME) + attach_volume(self, context, volume_id, instance, device) except Exception: with excutils.save_and_reraise_exception(): volume_bdm.destroy() @@ -3641,6 +3650,8 @@ class API(base.Base): attachment_id = None if attachments and instance.uuid in attachments: attachment_id = attachments[instance.uuid]['attachment_id'] + self._record_action_start( + context, instance, instance_actions.DETACH_VOLUME) self.compute_rpcapi.detach_volume(context, instance=instance, volume_id=volume['id'], attachment_id=attachment_id) @@ -3653,13 +3664,20 @@ class API(base.Base): If the volume has delete_on_termination option set then we call the volume api delete as well. """ + @wrap_instance_event(prefix='api') + def detach_volume(self, context, instance, bdms): + self._local_cleanup_bdm_volumes(bdms, instance, context) + try: self.volume_api.begin_detaching(context, volume['id']) except exception.InvalidInput as exc: raise exception.InvalidVolume(reason=exc.format_message()) bdms = [objects.BlockDeviceMapping.get_by_volume_id( context, volume['id'], instance.uuid)] - self._local_cleanup_bdm_volumes(bdms, instance, context) + self._record_action_start( + context, instance, + instance_actions.DETACH_VOLUME) + detach_volume(self, context, instance, bdms) @check_instance_lock @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.PAUSED, @@ -3712,6 +3730,9 @@ class API(base.Base): new_attachment_id = self.volume_api.attachment_create( context, new_volume['id'], instance.uuid)['id'] + self._record_action_start( + context, instance, instance_actions.SWAP_VOLUME) + try: self.compute_rpcapi.swap_volume( context, instance=instance, diff --git a/nova/compute/instance_actions.py b/nova/compute/instance_actions.py index 7b223e774fa3..58c1a0f0a4a1 100644 --- a/nova/compute/instance_actions.py +++ b/nova/compute/instance_actions.py @@ -62,3 +62,6 @@ TRIGGER_CRASH_DUMP = 'trigger_crash_dump' EXTEND_VOLUME = 'extend_volume' ATTACH_INTERFACE = 'attach_interface' DETACH_INTERFACE = 'detach_interface' +ATTACH_VOLUME = 'attach_volume' +DETACH_VOLUME = 'detach_volume' +SWAP_VOLUME = 'swap_volume' diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 0220a631cb13..868099f0cbd1 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -5093,6 +5093,7 @@ class ComputeManager(manager.Manager): return do_reserve() @wrap_exception() + @wrap_instance_event(prefix='compute') @wrap_instance_fault def attach_volume(self, context, instance, bdm): """Attach a volume to an instance.""" @@ -5235,6 +5236,7 @@ class ComputeManager(manager.Manager): instance=instance) @wrap_exception() + @wrap_instance_event(prefix='compute') @wrap_instance_fault def detach_volume(self, context, volume_id, instance, attachment_id=None): """Detach a volume from an instance. @@ -5399,6 +5401,7 @@ class ComputeManager(manager.Manager): return (comp_ret, new_cinfo) @wrap_exception() + @wrap_instance_event(prefix='compute') @wrap_instance_fault def swap_volume(self, context, old_volume_id, new_volume_id, instance, new_attachment_id=None): diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 3b6395aa35e6..c2d1412263ab 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -46,6 +46,7 @@ from nova import block_device from nova import compute from nova.compute import api as compute_api from nova.compute import flavors +from nova.compute import instance_actions from nova.compute import manager as compute_manager from nova.compute import power_state from nova.compute import rpcapi as compute_rpcapi @@ -393,7 +394,8 @@ class ComputeVolumeTestCase(BaseTestCase): self.stub_out('nova.db.block_device_mapping_create', store_cinfo) self.stub_out('nova.db.block_device_mapping_update', store_cinfo) - def test_attach_volume_serial(self): + @mock.patch.object(compute_utils, 'EventReporter') + def test_attach_volume_serial(self, mock_event): fake_bdm = objects.BlockDeviceMapping(context=self.context, **self.fake_volume) with (mock.patch.object(cinder.API, 'get_volume_encryption_metadata', @@ -401,10 +403,14 @@ class ComputeVolumeTestCase(BaseTestCase): instance = self._create_fake_instance_obj() self.compute.attach_volume(self.context, instance, bdm=fake_bdm) self.assertEqual(self.cinfo.get('serial'), uuids.volume_id) + mock_event.assert_called_once_with( + self.context, 'compute_attach_volume', instance.uuid) + @mock.patch.object(compute_utils, 'EventReporter') @mock.patch('nova.context.RequestContext.elevated') @mock.patch('nova.compute.utils.notify_about_volume_attach_detach') - def test_attach_volume_raises(self, mock_notify, mock_elevate): + def test_attach_volume_raises(self, mock_notify, mock_elevate, + mock_event): mock_elevate.return_value = self.context fake_bdm = objects.BlockDeviceMapping(**self.fake_volume) @@ -436,8 +442,11 @@ class ComputeVolumeTestCase(BaseTestCase): volume_id=uuids.volume_id, exception=expected_exception), ]) + mock_event.assert_called_once_with( + self.context, 'compute_attach_volume', instance.uuid) - def test_detach_volume_api_raises(self): + @mock.patch.object(compute_utils, 'EventReporter') + def test_detach_volume_api_raises(self, mock_event): fake_bdm = objects.BlockDeviceMapping(**self.fake_volume) instance = self._create_fake_instance_obj() @@ -454,8 +463,11 @@ class ComputeVolumeTestCase(BaseTestCase): test.TestingException, self.compute.detach_volume, self.context, 'fake', instance, 'fake_id') self.assertFalse(mock_destroy.called) + mock_event.assert_called_once_with( + self.context, 'compute_detach_volume', instance.uuid) - def test_detach_volume_bdm_destroyed(self): + @mock.patch.object(compute_utils, 'EventReporter') + def test_detach_volume_bdm_destroyed(self, mock_event): # Assert that the BDM is destroyed given a successful call to detach # the volume from the instance in Cinder. fake_bdm = objects.BlockDeviceMapping(**self.fake_volume) @@ -475,6 +487,8 @@ class ComputeVolumeTestCase(BaseTestCase): instance.uuid, uuids.attachment_id) self.assertTrue(mock_destroy.called) + mock_event.assert_called_once_with( + self.context, 'compute_detach_volume', instance.uuid) def test_await_block_device_created_too_slow(self): self.flags(block_device_allocate_retries=2) @@ -10068,8 +10082,9 @@ class ComputeAPITestCase(BaseTestCase): with test.nested( mock.patch.object(compute_api.API, '_check_attach_and_reserve_volume'), - mock.patch.object(cinder.API, 'attach') - ) as (mock_attach_and_reserve, mock_attach): + mock.patch.object(cinder.API, 'attach'), + mock.patch.object(compute_utils, 'EventReporter') + ) as (mock_attach_and_reserve, mock_attach, mock_event): self.compute_api._attach_volume_shelved_offloaded( self.context, instance, 'fake-volume-id', '/dev/vdb', 'ide', 'cdrom') @@ -10080,6 +10095,9 @@ class ComputeAPITestCase(BaseTestCase): 'fake-volume-id', instance.uuid, '/dev/vdb') + mock_event.assert_called_once_with( + self.context, 'api_attach_volume', instance.uuid + ) self.assertTrue(mock_attach.called) def test_attach_volume_no_device(self): @@ -10125,7 +10143,8 @@ class ComputeAPITestCase(BaseTestCase): self.assertTrue(called.get('fake_rpc_reserve_block_device_name')) self.assertTrue(called.get('fake_rpc_attach_volume')) - def test_detach_volume(self): + @mock.patch('nova.compute.api.API._record_action_start') + def test_detach_volume(self, mock_record): # Ensure volume can be detached from instance called = {} instance = self._create_fake_instance_obj() @@ -10147,14 +10166,19 @@ class ComputeAPITestCase(BaseTestCase): instance, volume) self.assertTrue(called.get('fake_begin_detaching')) self.assertTrue(called.get('fake_rpc_detach_volume')) + mock_record.assert_called_once_with( + self.context, instance, instance_actions.DETACH_VOLUME) + @mock.patch('nova.compute.api.API._record_action_start') + @mock.patch.object(compute_utils, 'EventReporter') @mock.patch.object(nova.volume.cinder.API, 'begin_detaching') @mock.patch.object(compute_api.API, '_local_cleanup_bdm_volumes') @mock.patch.object(objects.BlockDeviceMapping, 'get_by_volume_id') def test_detach_volume_shelved_offloaded(self, mock_block_dev, mock_local_cleanup, - mock_begin_detaching): + mock_begin_detaching, + mock_event, mock_record): mock_block_dev.return_value = [block_device_obj.BlockDeviceMapping( context=context)] @@ -10165,6 +10189,11 @@ class ComputeAPITestCase(BaseTestCase): volume) mock_begin_detaching.assert_called_once_with(self.context, volume['id']) + mock_record.assert_called_once_with( + self.context, instance, instance_actions.DETACH_VOLUME) + mock_event.assert_called_once_with(self.context, + 'api_detach_volume', + instance.uuid) self.assertTrue(mock_local_cleanup.called) @mock.patch.object(nova.volume.cinder.API, 'begin_detaching', diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 2b0001ea1b21..e817227effbf 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -381,9 +381,10 @@ class _ComputeAPIUnitTestMixIn(object): self.assertEqual(result.volume_id, bdm.volume_id) self.assertTrue(bdm_create.called) + @mock.patch.object(compute_api.API, '_record_action_start') @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name') @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') - def test_attach_volume(self, mock_attach, mock_reserve): + def test_attach_volume(self, mock_attach, mock_reserve, mock_record): instance = self._create_instance_obj() volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol', None, None, None, None, None) @@ -404,10 +405,14 @@ class _ComputeAPIUnitTestMixIn(object): volume['id']) mock_attach.assert_called_once_with(self.context, instance, fake_bdm) + mock_record.assert_called_once_with( + self.context, instance, instance_actions.ATTACH_VOLUME) + @mock.patch.object(compute_api.API, '_record_action_start') @mock.patch.object(compute_rpcapi.ComputeAPI, 'reserve_block_device_name') @mock.patch.object(compute_rpcapi.ComputeAPI, 'attach_volume') - def test_tagged_volume_attach(self, mock_attach, mock_reserve): + def test_tagged_volume_attach(self, mock_attach, mock_reserve, + mock_record): instance = self._create_instance_obj() volume = fake_volume.fake_volume(1, 'test-vol', 'test-vol', None, None, None, None, None) @@ -432,6 +437,8 @@ class _ComputeAPIUnitTestMixIn(object): volume['id']) mock_attach.assert_called_once_with(self.context, instance, fake_bdm) + mock_record.assert_called_once_with( + self.context, instance, instance_actions.ATTACH_VOLUME) def test_attach_volume_shelved_instance(self): instance = self._create_instance_obj() @@ -2307,6 +2314,7 @@ class _ComputeAPIUnitTestMixIn(object): if volumes[uuids.new_volume]['status'] == 'reserved': volumes[uuids.new_volume]['status'] = 'available' + @mock.patch.object(compute_api.API, '_record_action_start') @mock.patch.object(self.compute_api.compute_rpcapi, 'swap_volume', return_value=True) @mock.patch.object(self.compute_api.volume_api, 'unreserve_volume', @@ -2326,7 +2334,7 @@ class _ComputeAPIUnitTestMixIn(object): def _do_test(mock_begin_detaching, mock_get_by_volume_and_instance, mock_roll_detaching, mock_attachment_create, mock_reserve_volume, mock_attachment_delete, - mock_unreserve_volume, mock_swap_volume): + mock_unreserve_volume, mock_swap_volume, mock_record): bdm = objects.BlockDeviceMapping( **fake_block_device.FakeDbBlockDeviceDict( {'no_device': False, 'volume_id': '1', 'boot_index': 0, @@ -2383,6 +2391,9 @@ class _ComputeAPIUnitTestMixIn(object): old_volume_id=uuids.old_volume, new_volume_id=uuids.new_volume, new_attachment_id=attachment_id) + mock_record.assert_called_once_with(self.context, + instance, + instance_actions.SWAP_VOLUME) _do_test() diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index 352a162e257e..189f74d6b3f1 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -1832,6 +1832,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): do_test() + @mock.patch.object(compute_utils, 'EventReporter') @mock.patch.object(virt_driver.ComputeDriver, 'get_volume_connector', return_value={}) @mock.patch.object(manager.ComputeManager, '_instance_update', @@ -1854,6 +1855,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): mock_instance_fault_create, mock_instance_update, mock_get_volume_connector, + mock_event, expected_exception=None): # This test ensures that volume_id arguments are passed to volume_api # and that volume states are OK @@ -1955,6 +1957,9 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase): fields.NotificationAction.VOLUME_SWAP, fields.NotificationPhase.END, uuids.old_volume, uuids.new_volume) + mock_event.assert_called_once_with(self.context, + 'compute_swap_volume', + instance1.uuid) def _assert_volume_api(self, context, volume, *args): self.assertTrue(uuidutils.is_uuid_like(volume)) diff --git a/releasenotes/notes/fill-instance-action-record-gaps-14b36eba313d6d87.yaml b/releasenotes/notes/fill-instance-action-record-gaps-14b36eba313d6d87.yaml index 89cd5144d5be..ae4f0ff002e8 100644 --- a/releasenotes/notes/fill-instance-action-record-gaps-14b36eba313d6d87.yaml +++ b/releasenotes/notes/fill-instance-action-record-gaps-14b36eba313d6d87.yaml @@ -5,3 +5,6 @@ features: * attach_interface * detach_interface + * attach_volume + * detach_volume + * swap_volume