From a5c0e9574fe6030ed48488740ef2dea4288c866d Mon Sep 17 00:00:00 2001 From: Clinton Knight Date: Wed, 24 Feb 2016 17:59:36 -0500 Subject: [PATCH] Fix NetApp cDOT driver update_access negative test A negative internal Tempest test somehow didn't catch a problem with the recent update_access patch. If a flexvol is deleted on cDOT without Manila's knowledge, deleting the corresponding Manila share should still succeed. The core update_access changes call update_access during a share delete workflow to remove all the existing rules, and that path in the cDOT driver needs to be hardened against the cases where either share or share server no longer exist. Also, fix a mis-named lock object in the same code path. Change-Id: Ibb88c64ddd899c09cd148f398e21ac613be9f15b Closes-Bug: #1549518 --- .../netapp/dataontap/cluster_mode/lib_base.py | 23 +++++- .../netapp/dataontap/protocols/base.py | 2 +- .../dataontap/cluster_mode/test_lib_base.py | 73 +++++++++++++++++-- 3 files changed, 85 insertions(+), 13 deletions(-) diff --git a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py index 6062cfedfb..bf8b713ba0 100644 --- a/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py +++ b/manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py @@ -959,11 +959,26 @@ class NetAppCmodeFileStorageLibrary(object): def update_access(self, context, share, access_rules, add_rules=None, delete_rules=None, share_server=None): """Updates access rules for a share.""" - vserver, vserver_client = self._get_vserver(share_server=share_server) + try: + vserver, vserver_client = self._get_vserver( + share_server=share_server) + except (exception.InvalidInput, + exception.VserverNotSpecified, + exception.VserverNotFound) as error: + LOG.warning(_LW("Could not determine share server for share " + "%(share)s during access rules update. " + "Error: %(error)s"), + {'share': share['id'], 'error': error}) + return + share_name = self._get_valid_share_name(share['id']) - helper = self._get_helper(share) - helper.set_client(vserver_client) - helper.update_access(share, share_name, access_rules) + if self._share_exists(share_name, vserver_client): + helper = self._get_helper(share) + helper.set_client(vserver_client) + helper.update_access(share, share_name, access_rules) + else: + LOG.warning(_LW("Could not update access rules, share %s does " + "not exist."), share['id']) def setup_server(self, network_info, metadata=None): raise NotImplementedError() diff --git a/manila/share/drivers/netapp/dataontap/protocols/base.py b/manila/share/drivers/netapp/dataontap/protocols/base.py index fc202fa031..8089b05551 100644 --- a/manila/share/drivers/netapp/dataontap/protocols/base.py +++ b/manila/share/drivers/netapp/dataontap/protocols/base.py @@ -27,7 +27,7 @@ def access_rules_synchronized(f): def wrapped_func(self, *args, **kwargs): # The first argument is always a share, which has an ID - key = "cifs-access-%s" % args[0]['id'] + key = "share-access-%s" % args[0]['id'] @utils.synchronized(key) def source_func(self, *args, **kwargs): diff --git a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py index 85c8165ce0..945b01e1f7 100644 --- a/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py +++ b/manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py @@ -1745,27 +1745,84 @@ class NetAppFileStorageLibraryTestCase(test.TestCase): def test_update_access(self): + vserver_client = mock.Mock() + mock_get_vserver = self.mock_object( + self.library, '_get_vserver', + mock.Mock(return_value=(fake.VSERVER1, vserver_client))) protocol_helper = mock.Mock() protocol_helper.update_access.return_value = None self.mock_object(self.library, '_get_helper', mock.Mock(return_value=protocol_helper)) - vserver_client = mock.Mock() - self.mock_object(self.library, - '_get_vserver', - mock.Mock(return_value=(fake.VSERVER1, - vserver_client))) + mock_share_exists = self.mock_object(self.library, + '_share_exists', + mock.Mock(return_value=True)) self.library.update_access(self.context, fake.SHARE, [fake.SHARE_ACCESS], share_server=fake.SHARE_SERVER) + mock_get_vserver.assert_called_once_with( + share_server=fake.SHARE_SERVER) + share_name = self.library._get_valid_share_name(fake.SHARE['id']) + mock_share_exists.assert_called_once_with(share_name, vserver_client) protocol_helper.set_client.assert_called_once_with(vserver_client) protocol_helper.update_access.assert_called_once_with( - fake.SHARE, - fake.SHARE_NAME, - [fake.SHARE_ACCESS]) + fake.SHARE, fake.SHARE_NAME, [fake.SHARE_ACCESS]) + + @ddt.data(exception.InvalidInput(reason='fake_reason'), + exception.VserverNotSpecified(), + exception.VserverNotFound(vserver='fake_vserver')) + def test_update_access_no_share_server(self, get_vserver_exception): + + mock_get_vserver = self.mock_object( + self.library, '_get_vserver', + mock.Mock(side_effect=get_vserver_exception)) + protocol_helper = mock.Mock() + protocol_helper.update_access.return_value = None + self.mock_object(self.library, + '_get_helper', + mock.Mock(return_value=protocol_helper)) + mock_share_exists = self.mock_object(self.library, '_share_exists') + + self.library.update_access(self.context, + fake.SHARE, + [fake.SHARE_ACCESS], + share_server=fake.SHARE_SERVER) + + mock_get_vserver.assert_called_once_with( + share_server=fake.SHARE_SERVER) + self.assertFalse(mock_share_exists.called) + self.assertFalse(protocol_helper.set_client.called) + self.assertFalse(protocol_helper.update_access.called) + + def test_update_access_share_not_found(self): + + vserver_client = mock.Mock() + mock_get_vserver = self.mock_object( + self.library, '_get_vserver', + mock.Mock(return_value=(fake.VSERVER1, vserver_client))) + protocol_helper = mock.Mock() + protocol_helper.update_access.return_value = None + self.mock_object(self.library, + '_get_helper', + mock.Mock(return_value=protocol_helper)) + mock_share_exists = self.mock_object(self.library, + '_share_exists', + mock.Mock(return_value=False)) + + self.library.update_access(self.context, + fake.SHARE, + [fake.SHARE_ACCESS], + share_server=fake.SHARE_SERVER) + + mock_get_vserver.assert_called_once_with( + share_server=fake.SHARE_SERVER) + share_name = self.library._get_valid_share_name(fake.SHARE['id']) + mock_share_exists.assert_called_once_with(share_name, vserver_client) + self.assertFalse(protocol_helper.set_client.called) + self.assertFalse(protocol_helper.update_access.called) def test_setup_server(self): self.assertRaises(NotImplementedError,