From 6f506a87052e315f5f14c1edf6d592d782204b18 Mon Sep 17 00:00:00 2001 From: "mark.sturdevant" Date: Thu, 30 Jun 2016 16:04:44 -0700 Subject: [PATCH] HPE3PAR: Handle exceptions on deleted shares Fix to ensure that an attempt to delete a manila share will not fail because the 3PAR filestore does not exist. When the filestore does not exist, that means the share is already gone (or never was created). Avoid the delete and all filestore use in this case to prevent failures during delete. Also catch and log exceptions during the post-delete cleanup. Change-Id: I0e477d3dae5706547354bd029b40e3308349fabb Closes-Bug: #1597940 --- manila/share/drivers/hpe/hpe_3par_mediator.py | 45 ++-- .../drivers/hpe/test_hpe_3par_constants.py | 2 + .../drivers/hpe/test_hpe_3par_mediator.py | 233 ++++++++---------- ...hpe3par-delete-share-0daf75193f318c41.yaml | 3 + 4 files changed, 141 insertions(+), 142 deletions(-) create mode 100644 releasenotes/notes/bug-1597940-fix-hpe3par-delete-share-0daf75193f318c41.yaml diff --git a/manila/share/drivers/hpe/hpe_3par_mediator.py b/manila/share/drivers/hpe/hpe_3par_mediator.py index ced619fb72..cdbac26513 100644 --- a/manila/share/drivers/hpe/hpe_3par_mediator.py +++ b/manila/share/drivers/hpe/hpe_3par_mediator.py @@ -662,24 +662,35 @@ class HPE3ParMediator(object): if fstore: self._delete_share(share_name_ro, protocol, fpg, vfs, fstore) - if not self.hpe3par_fstore_per_share: - # Attempt to remove file tree on delete when using nested shares. - # If the file tree cannot be removed for whatever reason, we will - # not treat this as an error_deleting issue. We will allow the - # delete to continue as requested. - self._delete_file_tree(share_name, protocol, fpg, vfs, fstore) - if fstore: - # reduce the fsquota by share size when a share is deleted. - self._update_capacity_quotas(fstore, 0, share_size, fpg, vfs) + if fstore == share_name: + try: + self._client.removefstore(vfs, fstore, fpg=fpg) + except Exception as e: + msg = (_('Failed to remove fstore %(fstore)s: %(e)s') % + {'fstore': fstore, 'e': six.text_type(e)}) + LOG.exception(msg) + raise exception.ShareBackendException(msg=msg) - if fstore == share_name: - try: - self._client.removefstore(vfs, fstore, fpg=fpg) - except Exception as e: - msg = (_('Failed to remove fstore %(fstore)s: %(e)s') % - {'fstore': fstore, 'e': six.text_type(e)}) - LOG.exception(msg) - raise exception.ShareBackendException(msg=msg) + else: + try: + # Attempt to remove file tree on delete when using nested + # shares. If the file tree cannot be removed for whatever + # reason, we will not treat this as an error_deleting + # issue. We will allow the delete to continue as requested. + self._delete_file_tree( + share_name, protocol, fpg, vfs, fstore) + # reduce the fsquota by share size when a tree is deleted. + self._update_capacity_quotas( + fstore, 0, share_size, fpg, vfs) + except Exception as e: + msg = _LW('Exception during cleanup of deleted ' + 'share %(share)s in filestore %(fstore)s: %(e)s') + data = { + 'fstore': fstore, + 'share': share_name, + 'e': six.text_type(e), + } + LOG.warning(msg, data) def _delete_file_tree(self, share_name, protocol, fpg, vfs, fstore): # If the share protocol is CIFS, we need to make sure the admin diff --git a/manila/tests/share/drivers/hpe/test_hpe_3par_constants.py b/manila/tests/share/drivers/hpe/test_hpe_3par_constants.py index 66adb31ed7..500dfbdd32 100644 --- a/manila/tests/share/drivers/hpe/test_hpe_3par_constants.py +++ b/manila/tests/share/drivers/hpe/test_hpe_3par_constants.py @@ -40,7 +40,9 @@ EXPECTED_VLAN_TYPE = 'vlan' EXPECTED_VLAN_TAG = '101' EXPECTED_SERVER_ID = '1a1a1a1a-2b2b-3c3c-4d4d-5e5e5e5e5e5e' EXPECTED_PROJECT_ID = 'osf-nfs-project-id' +SHARE_ID = 'share-id' EXPECTED_SHARE_ID = 'osf-share-id' +EXPECTED_SHARE_ID_RO = 'osf-ro-share-id' EXPECTED_SHARE_NAME = 'share-name' EXPECTED_HOST = 'hostname@backend#pool' EXPECTED_SHARE_PATH = '/anyfpg/anyvfs/anyfstore' diff --git a/manila/tests/share/drivers/hpe/test_hpe_3par_mediator.py b/manila/tests/share/drivers/hpe/test_hpe_3par_mediator.py index 54cd563d5e..63646eefeb 100644 --- a/manila/tests/share/drivers/hpe/test_hpe_3par_mediator.py +++ b/manila/tests/share/drivers/hpe/test_hpe_3par_mediator.py @@ -599,21 +599,8 @@ class HPE3ParMediatorTestCase(test.TestCase): self.mock_object(self.mediator, '_find_fstore', mock.Mock(return_value=fstore)) - self.mock_object(self.mediator, - '_create_mount_directory', - mock.Mock(return_value={})) - self.mock_object(self.mediator, - '_mount_super_share', - mock.Mock(return_value={})) - self.mock_object(self.mediator, - '_delete_share_directory', - mock.Mock(return_value={})) - self.mock_object(self.mediator, - '_unmount_super_share', - mock.Mock(return_value={})) - self.mock_object(self.mediator, - '_update_capacity_quotas', - mock.Mock(return_value={})) + self.mock_object(self.mediator, '_delete_file_tree') + self.mock_object(self.mediator, '_update_capacity_quotas') self.mediator.delete_share(constants.EXPECTED_PROJECT_ID, share_id, @@ -633,35 +620,14 @@ class HPE3ParMediatorTestCase(test.TestCase): osf_ro_share_id, fpg=constants.EXPECTED_FPG, fstore=fstore), - mock.call.createfshare(constants.NFS_LOWER, - constants.EXPECTED_VFS, - constants.EXPECTED_SUPER_SHARE, - clientip=None, - comment=( - constants.EXPECTED_SUPER_SHARE_COMMENT), - fpg=constants.EXPECTED_FPG, - fstore=fstore, - options='rw,no_root_squash,insecure', - sharedir=''), mock.call.removefstore(constants.EXPECTED_VFS, fstore, fpg=constants.EXPECTED_FPG), ] self.mock_client.assert_has_calls(expected_calls) - expected_mount_path = constants.EXPECTED_MOUNT_PATH + osf_share_id - self.mediator._create_mount_directory.assert_called_with( - expected_mount_path) - self.mediator._mount_super_share.assert_called_with( - constants.NFS_LOWER, expected_mount_path, constants.EXPECTED_FPG, - constants.EXPECTED_VFS, fstore) - self.mediator._delete_share_directory.assert_called_with( - expected_mount_path) - self.mediator._unmount_super_share.assert_called_with( - expected_mount_path) - self.mediator._update_capacity_quotas.assert_called_with( - fstore, 0, constants.EXPECTED_SIZE_1, - constants.EXPECTED_FPG, constants.EXPECTED_VFS) + self.assertFalse(self.mediator._delete_file_tree.called) + self.assertFalse(self.mediator._update_capacity_quotas.called) def test_mediator_delete_share_not_found(self): self.init_mediator() @@ -669,18 +635,8 @@ class HPE3ParMediatorTestCase(test.TestCase): self.mock_object(self.mediator, '_find_fstore', mock.Mock(return_value=None)) - self.mock_object(self.mediator, - '_create_mount_directory', - mock.Mock(return_value={})) - self.mock_object(self.mediator, - '_mount_super_share', - mock.Mock(return_value={})) - self.mock_object(self.mediator, - '_delete_share_directory', - mock.Mock(return_value={})) - self.mock_object(self.mediator, - '_unmount_super_share', - mock.Mock(return_value={})) + self.mock_object(self.mediator, '_delete_file_tree') + self.mock_object(self.mediator, '_update_capacity_quotas') self.mediator.delete_share(constants.EXPECTED_PROJECT_ID, constants.EXPECTED_SHARE_ID, @@ -691,17 +647,8 @@ class HPE3ParMediatorTestCase(test.TestCase): self.assertFalse(self.mock_client.removefshare.called) - expected_mount_path = constants.EXPECTED_MOUNT_PATH + ( - constants.EXPECTED_SHARE_ID) - self.mediator._create_mount_directory.assert_called_with( - expected_mount_path) - self.mediator._mount_super_share.assert_called_with( - constants.SMB_LOWER, expected_mount_path, constants.EXPECTED_FPG, - constants.EXPECTED_VFS, None) - self.mediator._delete_share_directory.assert_called_with( - expected_mount_path) - self.mediator._unmount_super_share.assert_called_with( - expected_mount_path) + self.assertFalse(self.mediator._delete_file_tree.called) + self.assertFalse(self.mediator._update_capacity_quotas.called) def test_mediator_delete_nfs_share_only_readonly(self): self.init_mediator() @@ -779,28 +726,15 @@ class HPE3ParMediatorTestCase(test.TestCase): self.mock_object(self.mediator, '_find_fstore', mock.Mock(return_value=constants.EXPECTED_SHARE_ID)) - self.mock_object(self.mediator, - '_create_mount_directory', - mock.Mock(return_value={})) - self.mock_object(self.mediator, - '_mount_super_share', - mock.Mock(return_value={})) - self.mock_object(self.mediator, - '_delete_share_directory', - mock.Mock(return_value={})) - self.mock_object(self.mediator, - '_unmount_super_share', - mock.Mock(return_value={})) - self.mock_object(self.mediator, - '_update_capacity_quotas', - mock.Mock(return_value={})) + self.mock_object(self.mediator, '_delete_file_tree') + self.mock_object(self.mediator, '_update_capacity_quotas') self.mock_client.removefstore.side_effect = Exception( 'removefstore fail.') self.assertRaises(exception.ShareBackendException, self.mediator.delete_share, constants.EXPECTED_PROJECT_ID, - constants.EXPECTED_SHARE_ID, + constants.SHARE_ID, constants.EXPECTED_SIZE_1, constants.CIFS, constants.EXPECTED_FPG, @@ -812,51 +746,62 @@ class HPE3ParMediatorTestCase(test.TestCase): constants.EXPECTED_SHARE_ID, fpg=constants.EXPECTED_FPG, fstore=constants.EXPECTED_SHARE_ID), - mock.call.createfshare(constants.SMB_LOWER, + mock.call.removefshare(constants.SMB_LOWER, constants.EXPECTED_VFS, - constants.EXPECTED_SUPER_SHARE, - allowip=None, - comment=( - constants.EXPECTED_SUPER_SHARE_COMMENT), + constants.EXPECTED_SHARE_ID_RO, fpg=constants.EXPECTED_FPG, - fstore=constants.EXPECTED_SHARE_ID, - sharedir=''), - mock.call.setfshare(constants.SMB_LOWER, - constants.EXPECTED_VFS, - constants.EXPECTED_SUPER_SHARE, - comment=( - constants.EXPECTED_SUPER_SHARE_COMMENT), - allowperm=( - '+' + constants.USERNAME + ':fullcontrol'), - fpg=constants.EXPECTED_FPG, - fstore=constants.EXPECTED_SHARE_ID), + fstore=constants.EXPECTED_SHARE_ID), mock.call.removefstore(constants.EXPECTED_VFS, constants.EXPECTED_SHARE_ID, fpg=constants.EXPECTED_FPG), ] self.mock_client.assert_has_calls(expected_calls) - expected_mount_path = constants.EXPECTED_MOUNT_PATH + ( - constants.EXPECTED_SHARE_ID) - self.mediator._create_mount_directory.assert_called_with( - expected_mount_path) - self.mediator._mount_super_share.assert_called_with( - constants.SMB_LOWER, expected_mount_path, constants.EXPECTED_FPG, - constants.EXPECTED_VFS, constants.EXPECTED_SHARE_ID) - self.mediator._delete_share_directory.assert_called_with( - expected_mount_path) - self.mediator._unmount_super_share.assert_called_with( - expected_mount_path) - self.mediator._update_capacity_quotas.assert_called_with( - constants.EXPECTED_SHARE_ID, 0, constants.EXPECTED_SIZE_1, - constants.EXPECTED_FPG, constants.EXPECTED_VFS) + self.assertFalse(self.mediator._delete_file_tree.called) + self.assertFalse(self.mediator._update_capacity_quotas.called) + + def test_mediator_delete_file_tree_exception(self): + self.init_mediator() + mock_log = self.mock_object(hpe3parmediator, 'LOG') + self.mock_object(self.mediator, + '_find_fstore', + mock.Mock(return_value=constants.EXPECTED_FSTORE)) + self.mock_object(self.mediator, + '_delete_file_tree', + mock.Mock(side_effect=Exception('test'))) + self.mock_object(self.mediator, '_update_capacity_quotas') + + self.mediator.delete_share(constants.EXPECTED_PROJECT_ID, + constants.SHARE_ID, + constants.EXPECTED_SIZE_1, + constants.CIFS, + constants.EXPECTED_FPG, + constants.EXPECTED_VFS) + + expected_calls = [ + mock.call.removefshare(constants.SMB_LOWER, + constants.EXPECTED_VFS, + constants.EXPECTED_SHARE_ID, + fpg=constants.EXPECTED_FPG, + fstore=constants.EXPECTED_FSTORE), + mock.call.removefshare(constants.SMB_LOWER, + constants.EXPECTED_VFS, + constants.EXPECTED_SHARE_ID_RO, + fpg=constants.EXPECTED_FPG, + fstore=constants.EXPECTED_FSTORE), + ] + self.mock_client.assert_has_calls(expected_calls) + + self.assertTrue(self.mediator._delete_file_tree.called) + self.assertFalse(self.mediator._update_capacity_quotas.called) + mock_log.warning.assert_called_once_with(mock.ANY, mock.ANY) def test_mediator_delete_cifs_share(self): self.init_mediator() self.mock_object(self.mediator, '_find_fstore', - mock.Mock(return_value=constants.EXPECTED_SHARE_ID)) + mock.Mock(return_value=constants.EXPECTED_FSTORE)) self.mock_object(self.mediator, '_create_mount_directory', mock.Mock(return_value={})) @@ -885,7 +830,7 @@ class HPE3ParMediatorTestCase(test.TestCase): constants.EXPECTED_VFS, constants.EXPECTED_SHARE_ID, fpg=constants.EXPECTED_FPG, - fstore=constants.EXPECTED_SHARE_ID), + fstore=constants.EXPECTED_FSTORE), mock.call.createfshare(constants.SMB_LOWER, constants.EXPECTED_VFS, constants.EXPECTED_SUPER_SHARE, @@ -893,7 +838,7 @@ class HPE3ParMediatorTestCase(test.TestCase): comment=( constants.EXPECTED_SUPER_SHARE_COMMENT), fpg=constants.EXPECTED_FPG, - fstore=constants.EXPECTED_SHARE_ID, + fstore=constants.EXPECTED_FSTORE, sharedir=''), mock.call.setfshare(constants.SMB_LOWER, constants.EXPECTED_VFS, @@ -903,27 +848,65 @@ class HPE3ParMediatorTestCase(test.TestCase): allowperm=( '+' + constants.USERNAME + ':fullcontrol'), fpg=constants.EXPECTED_FPG, - fstore=constants.EXPECTED_SHARE_ID), + fstore=constants.EXPECTED_FSTORE), + ] + self.mock_client.assert_has_calls(expected_calls) + + expected_mount_path = constants.EXPECTED_MOUNT_PATH + ( + constants.EXPECTED_SHARE_ID) + expected_share_path = '/'.join((expected_mount_path, + constants.EXPECTED_SHARE_ID)) + self.mediator._create_mount_directory.assert_called_once_with( + expected_mount_path) + self.mediator._mount_super_share.assert_called_once_with( + constants.SMB_LOWER, + expected_mount_path, + constants.EXPECTED_FPG, + constants.EXPECTED_VFS, + constants.EXPECTED_FSTORE) + self.mediator._delete_share_directory.assert_has_calls([ + mock.call(expected_share_path), + mock.call(expected_mount_path), + ]) + self.mediator._unmount_super_share.assert_called_once_with( + expected_mount_path) + self.mediator._update_capacity_quotas.assert_called_once_with( + constants.EXPECTED_FSTORE, + 0, + constants.EXPECTED_SIZE_1, + constants.EXPECTED_FPG, + constants.EXPECTED_VFS) + + def test_mediator_delete_cifs_share_and_fstore(self): + self.init_mediator() + + self.mock_object(self.mediator, + '_find_fstore', + mock.Mock(return_value=constants.EXPECTED_SHARE_ID)) + self.mock_object(self.mediator, '_delete_file_tree') + self.mock_object(self.mediator, '_update_capacity_quotas') + + self.mediator.delete_share(constants.EXPECTED_PROJECT_ID, + constants.EXPECTED_SHARE_ID, + constants.EXPECTED_SIZE_1, + constants.CIFS, + constants.EXPECTED_FPG, + constants.EXPECTED_VFS) + + expected_calls = [ + mock.call.removefshare(constants.SMB_LOWER, + constants.EXPECTED_VFS, + constants.EXPECTED_SHARE_ID, + fpg=constants.EXPECTED_FPG, + fstore=constants.EXPECTED_SHARE_ID), mock.call.removefstore(constants.EXPECTED_VFS, constants.EXPECTED_SHARE_ID, fpg=constants.EXPECTED_FPG), ] self.mock_client.assert_has_calls(expected_calls) - expected_mount_path = constants.EXPECTED_MOUNT_PATH + ( - constants.EXPECTED_SHARE_ID) - self.mediator._create_mount_directory.assert_called_with( - expected_mount_path) - self.mediator._mount_super_share.assert_called_with( - constants.SMB_LOWER, expected_mount_path, constants.EXPECTED_FPG, - constants.EXPECTED_VFS, constants.EXPECTED_SHARE_ID) - self.mediator._delete_share_directory.assert_called_with( - expected_mount_path) - self.mediator._unmount_super_share.assert_called_with( - expected_mount_path) - self.mediator._update_capacity_quotas.assert_called_with( - constants.EXPECTED_SHARE_ID, 0, constants.EXPECTED_SIZE_1, - constants.EXPECTED_FPG, constants.EXPECTED_VFS) + self.assertFalse(self.mediator._delete_file_tree.called) + self.assertFalse(self.mediator._update_capacity_quotas.called) def test_mediator_delete_share_with_fstore_per_share_false(self): self.init_mediator() diff --git a/releasenotes/notes/bug-1597940-fix-hpe3par-delete-share-0daf75193f318c41.yaml b/releasenotes/notes/bug-1597940-fix-hpe3par-delete-share-0daf75193f318c41.yaml new file mode 100644 index 0000000000..0acc51ce3e --- /dev/null +++ b/releasenotes/notes/bug-1597940-fix-hpe3par-delete-share-0daf75193f318c41.yaml @@ -0,0 +1,3 @@ +--- +fixes: + - HPE3PAR driver fix to allow delete of a share that does not exist on the backend.