From 4163fb29add1f3c80cb902c8735be3dde24930d3 Mon Sep 17 00:00:00 2001 From: Tom Barron Date: Mon, 14 Sep 2015 17:38:40 -0400 Subject: [PATCH] NetApp volume/snapshot delete performance fix cDOT NFS NetApp drivers back cinder volumes with array-side files and use array-side cloning techniques when creating snapshots and when creating volumes from glance images. Customers have experienced performance issues on the array when deleting many of these in in quick succession, especially for large volumes. This commit addresses these performance issues by overriding parent class volume and snapshot delete methods, which use OS "rm" command to delete volume backing files, with methods that use DOT API to delete these files, and which invoke the optimized file deletion engine available in DOT 8.3. Closes-bug: 1497258 Change-Id: I44428d0840f6584f93ca214a2a607869b345554c --- cinder/tests/unit/test_netapp_nfs.py | 42 ----- .../volume/drivers/netapp/dataontap/fakes.py | 36 ++++ .../netapp/dataontap/test_nfs_cmode.py | 161 +++++++++++++++--- .../netapp/dataontap/client/client_base.py | 5 + .../netapp/dataontap/client/client_cmode.py | 21 +++ .../drivers/netapp/dataontap/nfs_cmode.py | 52 +++++- 6 files changed, 249 insertions(+), 68 deletions(-) diff --git a/cinder/tests/unit/test_netapp_nfs.py b/cinder/tests/unit/test_netapp_nfs.py index f1f8c7efa..4479651d1 100644 --- a/cinder/tests/unit/test_netapp_nfs.py +++ b/cinder/tests/unit/test_netapp_nfs.py @@ -218,48 +218,6 @@ class NetAppCmodeNfsDriverTestCase(test.TestCase): mox.VerifyAll() - def _prepare_delete_snapshot_mock(self, snapshot_exists): - drv = self._driver - mox = self.mox - - mox.StubOutWithMock(drv, '_get_provider_location') - mox.StubOutWithMock(drv, '_volume_not_present') - mox.StubOutWithMock(drv, '_post_prov_deprov_in_ssc') - - if snapshot_exists: - mox.StubOutWithMock(drv, '_execute') - mox.StubOutWithMock(drv, '_get_volume_path') - drv._get_provider_location(mox_lib.IgnoreArg()) - drv._get_provider_location(mox_lib.IgnoreArg()) - drv._volume_not_present(mox_lib.IgnoreArg(), mox_lib.IgnoreArg())\ - .AndReturn(not snapshot_exists) - - if snapshot_exists: - drv._get_volume_path(mox_lib.IgnoreArg(), mox_lib.IgnoreArg()) - drv._execute('rm', None, run_as_root=True) - - drv._post_prov_deprov_in_ssc(mox_lib.IgnoreArg()) - - mox.ReplayAll() - - return mox - - def test_delete_existing_snapshot(self): - drv = self._driver - mox = self._prepare_delete_snapshot_mock(True) - - drv.delete_snapshot(FakeSnapshot()) - - mox.VerifyAll() - - def test_delete_missing_snapshot(self): - drv = self._driver - mox = self._prepare_delete_snapshot_mock(False) - - drv.delete_snapshot(FakeSnapshot()) - - mox.VerifyAll() - @mock.patch.object(nfs_base.NetAppNfsDriver, 'do_setup') @mock.patch.object(client_cmode.Client, '__init__', return_value=None) def test_do_setup(self, mock_client_init, mock_super_do_setup): diff --git a/cinder/tests/unit/volume/drivers/netapp/dataontap/fakes.py b/cinder/tests/unit/volume/drivers/netapp/dataontap/fakes.py index 472f58e73..b7f61ff97 100644 --- a/cinder/tests/unit/volume/drivers/netapp/dataontap/fakes.py +++ b/cinder/tests/unit/volume/drivers/netapp/dataontap/fakes.py @@ -238,3 +238,39 @@ FAKE_LUN = netapp_api.NaElement.create_node_with_children( 'uuid': 'cec1f3d7-3d41-11e2-9cf4-123478563412', 'volume': 'fakeLUN', 'vserver': 'fake_vserver'}) + + +class test_volume(object): + pass + +test_volume = test_volume() +test_volume.id = {'vserver': 'openstack', 'name': 'vola'} +test_volume.aggr = { + 'disk_type': 'SSD', + 'ha_policy': 'cfo', + 'junction': '/vola', + 'name': 'aggr1', + 'raid_type': 'raiddp', +} +test_volume.export = {'path': NFS_SHARE} +test_volume.sis = {'dedup': False, 'compression': False} +test_volume.state = { + 'status': 'online', + 'vserver_root': False, + 'junction_active': True, +} +test_volume.qos = {'qos_policy_group': None} + + +class test_snapshot(object): + pass + + def __getitem__(self, key): + return getattr(self, key) + +PROVIDER_LOCATION = 'fake_provider_location' +test_snapshot = test_snapshot() +test_snapshot.id = 'fake_snap_id' +test_snapshot.name = 'snapshot-%s' % test_snapshot.id +test_snapshot.volume_id = 'fake_volume_id' +test_snapshot.provider_location = PROVIDER_LOCATION diff --git a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_cmode.py b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_cmode.py index 13536fb33..0614b83a8 100644 --- a/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_cmode.py +++ b/cinder/tests/unit/volume/drivers/netapp/dataontap/test_nfs_cmode.py @@ -178,39 +178,113 @@ class NetAppCmodeNfsDriverTestCase(test.TestCase): def test_delete_volume(self): fake_provider_location = 'fake_provider_location' - fake_volume = {'name': 'fake_name', - 'provider_location': 'fake_provider_location'} - fake_qos_policy_group_info = {'legacy': None, 'spec': None} - self.mock_object(nfs_base.NetAppNfsDriver, 'delete_volume') - self.mock_object(na_utils, 'get_valid_qos_policy_group_info', - mock.Mock(return_value=fake_qos_policy_group_info)) - self.mock_object(self.driver, '_post_prov_deprov_in_ssc') + fake_volume = {'provider_location': fake_provider_location} + self.mock_object(self.driver, '_delete_backing_file_for_volume') + self.mock_object(na_utils, 'get_valid_qos_policy_group_info') self.driver.zapi_client = mock.Mock() + mock_prov_deprov = self.mock_object(self.driver, + '_post_prov_deprov_in_ssc') self.driver.delete_volume(fake_volume) - nfs_base.NetAppNfsDriver.delete_volume.assert_called_once_with( - fake_volume) - self.driver.zapi_client.mark_qos_policy_group_for_deletion\ - .assert_called_once_with(fake_qos_policy_group_info) - self.driver._post_prov_deprov_in_ssc.assert_called_once_with( - fake_provider_location) + mock_prov_deprov.assert_called_once_with(fake_provider_location) - def test_delete_volume_get_qos_info_exception(self): + def test_delete_volume_exception_path(self): fake_provider_location = 'fake_provider_location' - fake_volume = {'name': 'fake_name', - 'provider_location': 'fake_provider_location'} - self.mock_object(nfs_base.NetAppNfsDriver, 'delete_volume') - self.mock_object(na_utils, 'get_valid_qos_policy_group_info', - mock.Mock(side_effect=exception.Invalid)) - self.mock_object(self.driver, '_post_prov_deprov_in_ssc') + fake_volume = {'provider_location': fake_provider_location} + self.mock_object(self.driver, '_delete_backing_file_for_volume') + self.mock_object(na_utils, 'get_valid_qos_policy_group_info') + self.driver.zapi_client = mock.Mock(side_effect=[Exception]) + mock_prov_deprov = self.mock_object(self.driver, + '_post_prov_deprov_in_ssc') self.driver.delete_volume(fake_volume) - nfs_base.NetAppNfsDriver.delete_volume.assert_called_once_with( - fake_volume) - self.driver._post_prov_deprov_in_ssc.assert_called_once_with( - fake_provider_location) + mock_prov_deprov.assert_called_once_with(fake_provider_location) + + def test_delete_backing_file_for_volume(self): + mock_filer_delete = self.mock_object(self.driver, + '_delete_volume_on_filer') + mock_super_delete = self.mock_object(nfs_base.NetAppNfsDriver, + 'delete_volume') + + self.driver._delete_backing_file_for_volume(fake.NFS_VOLUME) + + mock_filer_delete.assert_called_once_with(fake.NFS_VOLUME) + self.assertEqual(0, mock_super_delete.call_count) + + def test_delete_backing_file_for_volume_exception_path(self): + mock_filer_delete = self.mock_object(self.driver, + '_delete_volume_on_filer') + mock_filer_delete.side_effect = [Exception] + mock_super_delete = self.mock_object(nfs_base.NetAppNfsDriver, + 'delete_volume') + + self.driver._delete_backing_file_for_volume(fake.NFS_VOLUME) + + mock_filer_delete.assert_called_once_with(fake.NFS_VOLUME) + mock_super_delete.assert_called_once_with(fake.NFS_VOLUME) + + def test_delete_volume_on_filer(self): + mock_get_vs_ip = self.mock_object(self.driver, '_get_export_ip_path') + mock_get_vs_ip.return_value = (fake.VSERVER_NAME, '/%s' % fake.FLEXVOL) + self.driver.zapi_client = mock.Mock() + mock_zapi_delete = self.driver.zapi_client.delete_file + + self.driver._delete_volume_on_filer(fake.NFS_VOLUME) + + mock_zapi_delete.assert_called_once_with( + '/vol/%s/%s' % (fake.FLEXVOL, fake.NFS_VOLUME['name'])) + + def test_delete_snapshot(self): + mock_get_location = self.mock_object(self.driver, + '_get_provider_location') + mock_get_location.return_value = fake.PROVIDER_LOCATION + + mock_delete_backing = self.mock_object( + self.driver, '_delete_backing_file_for_snapshot') + + mock_prov_deprov = self.mock_object(self.driver, + '_post_prov_deprov_in_ssc') + + self.driver.delete_snapshot(fake.test_snapshot) + + mock_delete_backing.assert_called_once_with(fake.test_snapshot) + mock_prov_deprov.assert_called_once_with(fake.PROVIDER_LOCATION) + + def test_delete_backing_file_for_snapshot(self): + mock_filer_delete = self.mock_object( + self.driver, '_delete_snapshot_on_filer') + mock_super_delete = self.mock_object(nfs_base.NetAppNfsDriver, + 'delete_snapshot') + + self.driver._delete_backing_file_for_snapshot(fake.test_snapshot) + + mock_filer_delete.assert_called_once_with(fake.test_snapshot) + self.assertEqual(0, mock_super_delete.call_count) + + def test_delete_backing_file_for_snapshot_exception_path(self): + mock_filer_delete = self.mock_object( + self.driver, '_delete_snapshot_on_filer') + mock_filer_delete.side_effect = [Exception] + mock_super_delete = self.mock_object(nfs_base.NetAppNfsDriver, + 'delete_snapshot') + + self.driver._delete_backing_file_for_snapshot(fake.test_snapshot) + + mock_filer_delete.assert_called_once_with(fake.test_snapshot) + mock_super_delete.assert_called_once_with(fake.test_snapshot) + + def test_delete_snapshot_on_filer(self): + mock_get_vs_ip = self.mock_object(self.driver, '_get_export_ip_path') + mock_get_vs_ip.return_value = (fake.VSERVER_NAME, '/%s' % fake.FLEXVOL) + self.driver.zapi_client = mock.Mock() + mock_zapi_delete = self.driver.zapi_client.delete_file + + self.driver._delete_snapshot_on_filer(fake.test_snapshot) + + mock_zapi_delete.assert_called_once_with( + '/vol/%s/%s' % (fake.FLEXVOL, fake.test_snapshot['name'])) def test_do_qos_for_volume_no_exception(self): @@ -498,3 +572,42 @@ class NetAppCmodeNfsDriverTestCase(test.TestCase): fake_volume) self.assertEqual(expected, result) + + @ddt.data( + {'ssc': True, 'share': fake.NFS_SHARE, 'vol': fake.test_volume}, + {'ssc': True, 'share': fake.NFS_SHARE, 'vol': None}, + {'ssc': True, 'share': None, 'vol': fake.test_volume}, + {'ssc': True, 'share': None, 'vol': None}, + {'ssc': False, 'share': fake.NFS_SHARE, 'vol': fake.test_volume}, + {'ssc': False, 'share': fake.NFS_SHARE, 'vol': None}, + {'ssc': False, 'share': None, 'vol': fake.test_volume}, + {'ssc': False, 'share': None, 'vol': None}, + ) + @ddt.unpack + def test_post_prov_deprov_in_ssc(self, ssc, share, vol): + + with mock.patch.object(self.driver, 'ssc_enabled', ssc): + with mock.patch.object( + self.driver, '_get_vol_for_share') as mock_get_vol: + with mock.patch.object( + self.driver, '_update_stale_vols') as mock_update: + mock_get_vol.return_value = vol + self.driver._post_prov_deprov_in_ssc(share) + + if ssc and share and vol: + mock_update.assert_called_once_with(volume=vol) + else: + self.assertEqual(0, mock_update.call_count) + + def test_get_vol_for_share(self): + fake_volume = fake.test_volume + ssc_vols = {'all': {fake_volume}} + + with mock.patch.object(self.driver, 'ssc_vols', ssc_vols): + result = self.driver._get_vol_for_share(fake.NFS_SHARE) + + self.assertEqual(fake.test_volume, result) + + def test_get_vol_for_share_no_ssc_vols(self): + with mock.patch.object(self.driver, 'ssc_vols', None): + self.assertIsNone(self.driver._get_vol_for_share(fake.NFS_SHARE)) diff --git a/cinder/volume/drivers/netapp/dataontap/client/client_base.py b/cinder/volume/drivers/netapp/dataontap/client/client_base.py index 055f55dde..b37353dea 100644 --- a/cinder/volume/drivers/netapp/dataontap/client/client_base.py +++ b/cinder/volume/drivers/netapp/dataontap/client/client_base.py @@ -27,6 +27,7 @@ import six from cinder.i18n import _LE, _LW, _LI from cinder import utils +from cinder.volume.drivers.netapp import utils as na_utils netapp_lib = importutils.try_import('netapp_lib') if netapp_lib: @@ -47,6 +48,10 @@ class Client(object): username=kwargs['username'], password=kwargs['password']) + def _init_features(self): + """Set up the repository of available Data ONTAP features.""" + self.features = na_utils.Features() + def get_ontapi_version(self, cached=True): """Gets the supported ontapi version.""" diff --git a/cinder/volume/drivers/netapp/dataontap/client/client_cmode.py b/cinder/volume/drivers/netapp/dataontap/client/client_cmode.py index ce199bf65..f631dd4da 100644 --- a/cinder/volume/drivers/netapp/dataontap/client/client_cmode.py +++ b/cinder/volume/drivers/netapp/dataontap/client/client_cmode.py @@ -50,6 +50,15 @@ class Client(client_base.Client): self.connection.set_api_version(1, 15) (major, minor) = self.get_ontapi_version(cached=False) self.connection.set_api_version(major, minor) + self._init_features() + + def _init_features(self): + super(Client, self)._init_features() + + ontapi_version = self.get_ontapi_version() # major, minor + + ontapi_1_30 = ontapi_version >= (1, 30) + self.features.add_feature('FAST_CLONE_DELETE', supported=ontapi_1_30) def _invoke_vserver_api(self, na_element, vserver): server = copy.copy(self.connection) @@ -618,3 +627,15 @@ class Client(client_base.Client): volume_space_attributes.get_child_content('size-total')) return size_total, size_available + + @utils.trace_method + def delete_file(self, path_to_file): + """Delete file at path.""" + + api_args = { + 'path': path_to_file, + } + # Use fast clone deletion engine if it is supported. + if self.features.FAST_CLONE_DELETE: + api_args['is-clone-file'] = 'true' + self.send_request('file-delete-file', api_args, True) diff --git a/cinder/volume/drivers/netapp/dataontap/nfs_cmode.py b/cinder/volume/drivers/netapp/dataontap/nfs_cmode.py index 4d54de19f..b03b03b2d 100644 --- a/cinder/volume/drivers/netapp/dataontap/nfs_cmode.py +++ b/cinder/volume/drivers/netapp/dataontap/nfs_cmode.py @@ -331,7 +331,7 @@ class NetAppCmodeNfsDriver(nfs_base.NetAppNfsDriver): def delete_volume(self, volume): """Deletes a logical volume.""" share = volume['provider_location'] - super(NetAppCmodeNfsDriver, self).delete_volume(volume) + self._delete_backing_file_for_volume(volume) try: qos_policy_group_info = na_utils.get_valid_qos_policy_group_info( volume) @@ -343,12 +343,60 @@ class NetAppCmodeNfsDriver(nfs_base.NetAppNfsDriver): pass self._post_prov_deprov_in_ssc(share) + def _delete_backing_file_for_volume(self, volume): + """Deletes file on nfs share that backs a cinder volume.""" + try: + LOG.debug('Deleting backing file for volume %s.', volume['id']) + self._delete_volume_on_filer(volume) + except Exception: + LOG.exception(_LE('Could not do delete of volume %s on filer, ' + 'falling back to exec of "rm" command.'), + volume['id']) + try: + super(NetAppCmodeNfsDriver, self).delete_volume(volume) + except Exception: + LOG.exception(_LE('Exec of "rm" command on backing file for ' + '%s was unsuccessful.'), volume['id']) + + def _delete_volume_on_filer(self, volume): + (_vserver, flexvol) = self._get_export_ip_path(volume_id=volume['id']) + path_on_filer = '/vol' + flexvol + '/' + volume['name'] + LOG.debug('Attempting to delete backing file %s for volume %s on ' + 'filer.', path_on_filer, volume['id']) + self.zapi_client.delete_file(path_on_filer) + + @utils.trace_method def delete_snapshot(self, snapshot): """Deletes a snapshot.""" share = self._get_provider_location(snapshot.volume_id) - super(NetAppCmodeNfsDriver, self).delete_snapshot(snapshot) + self._delete_backing_file_for_snapshot(snapshot) self._post_prov_deprov_in_ssc(share) + @utils.trace_method + def _delete_backing_file_for_snapshot(self, snapshot): + """Deletes file on nfs share that backs a cinder volume.""" + try: + LOG.debug('Deleting backing file for snapshot %s.', snapshot['id']) + self._delete_snapshot_on_filer(snapshot) + except Exception: + LOG.exception(_LE('Could not do delete of snapshot %s on filer, ' + 'falling back to exec of "rm" command.'), + snapshot['id']) + try: + super(NetAppCmodeNfsDriver, self).delete_snapshot(snapshot) + except Exception: + LOG.exception(_LE('Exec of "rm" command on backing file for' + ' %s was unsuccessful.'), snapshot['id']) + + @utils.trace_method + def _delete_snapshot_on_filer(self, snapshot): + (_vserver, flexvol) = self._get_export_ip_path( + volume_id=snapshot['volume_id']) + path_on_filer = '/vol' + flexvol + '/' + snapshot['name'] + LOG.debug('Attempting to delete backing file %s for snapshot %s ' + 'on filer.', path_on_filer, snapshot['id']) + self.zapi_client.delete_file(path_on_filer) + def _post_prov_deprov_in_ssc(self, share): if self.ssc_enabled and share: netapp_vol = self._get_vol_for_share(share) -- 2.45.2