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

diff --git a/cinder/tests/unit/test_netapp_nfs.py b/cinder/tests/unit/test_netapp_nfs.py
index f1f8c7efa8f8388c773fbd467c2d2280965e2ae4..4479651d17c07be01d1b25f37fa3a86e80173dc3 100644 (file)
--- 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 472f58e73e5ecd3a5e0405b165a6c65f8a305e29..b7f61ff97a8f6a6bc0661bf8a72c57b3a9a1ac42 100644 (file)
--- 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 13536fb33dfdefe9df45cfe799e24f35ed0e7639..0614b83a8863929d62231d53086d46639f5b9981 100644 (file)
--- 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 055f55dde5ae922d51e98b00b0f85334b146d02e..b37353dea9fc22fc959bf7f2df16a40195627fcc 100644 (file)
--- 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 ce199bf656b0df8d91d5ca978df4bed6e97fe4fd..f631dd4da6fbfc4a27e686188d38deca9de6c2a9 100644 (file)
--- 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 4d54de19f8dee587abe21f2b9f6c638cb442fe18..b03b03b2dff9bec4b5de06ccec134b8c1f1fbec3 100644 (file)
--- 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)