Fix share-server-delete command

and add functional tests for it.
Which can be enabled/disabled using
'run_share_servers_tests' config option.

Change-Id: I07c5b03d576b41a61867a90d828a112c8a3f4f4b
Closes-Bug: #1629317

manilaclient/config.py

@@ -130,6 +130,12 @@ share_opts = [
                 help="Defines whether to run tests that use share snapshots "
                      "or not. Disable this feature if used driver doesn't "
                      "support it."),
+    cfg.BoolOpt("run_share_servers_tests",
+                default=True,
+                help="Defines whether to run tests that use share servers "
+                     "or not. Disable this feature if used driver doesn't "
+                     "support it or when autodeletion of share servers "
+                     "is enabled."),
 ]
 
 # 2. Generate config

manilaclient/tests/functional/client.py

@@ -31,6 +31,7 @@ CONF = config.CONF
 SHARE = 'share'
 SHARE_TYPE = 'share_type'
 SHARE_NETWORK = 'share_network'
+SHARE_SERVER = 'share_server'
 
 
 def not_found_wrapper(f):
@@ -39,9 +40,10 @@ def not_found_wrapper(f):
         try:
             return f(self, *args, **kwargs)
         except tempest_lib_exc.CommandFailed as e:
-            if re.search('No (\w+) with a name or ID', e.stderr):
-                # Raise appropriate 'NotFound' error
-                raise tempest_lib_exc.NotFound()
+            for regexp in ('No (\w+) with a name or ID', 'not found'):
+                if re.search(regexp, e.stderr):
+                    # Raise appropriate 'NotFound' error
+                    raise tempest_lib_exc.NotFound()
             raise
 
     return wrapped_func
@@ -123,6 +125,8 @@ class ManilaCLIClient(base.CLIClient):
             func = self.is_share_type_deleted
         elif res_type == SHARE_NETWORK:
             func = self.is_share_network_deleted
+        elif res_type == SHARE_SERVER:
+            func = self.is_share_server_deleted
         elif res_type == SHARE:
             func = self.is_share_deleted
         elif res_type == SNAPSHOT:
@@ -1068,3 +1072,64 @@ class ManilaCLIClient(base.CLIClient):
             microversion=microversion)
         share = output_parser.details(share_raw)
         return share
+
+    # Share servers
+
+    @not_found_wrapper
+    def get_share_server(self, share_server, microversion=None):
+        """Returns share server by its Name or ID."""
+        share_server_raw = self.manila(
+            'share-server-show %s' % share_server, microversion=microversion)
+        share_server = output_parser.details(share_server_raw)
+        return share_server
+
+    def list_share_servers(self, filters=None, columns=None,
+                           microversion=None):
+        """List share servers.
+
+        :param filters: dict -- filters for listing of share servers.
+            Example, input:
+                {'project_id': 'foo'}
+                {'-project_id': 'foo'}
+                {'--project_id': 'foo'}
+                {'project-id': 'foo'}
+            will be transformed to filter parameter "--project-id=foo"
+         :param columns: comma separated string of columns.
+            Example, "--columns id"
+        """
+        cmd = 'share-server-list '
+        if columns is not None:
+            cmd += ' --columns ' + columns
+        if filters and isinstance(filters, dict):
+            for k, v in filters.items():
+                cmd += '%(k)s=%(v)s ' % {
+                    'k': self._stranslate_to_cli_optional_param(k), 'v': v}
+        share_servers_raw = self.manila(cmd, microversion=microversion)
+        share_servers = utils.listing(share_servers_raw)
+        return share_servers
+
+    @not_found_wrapper
+    def delete_share_server(self, share_server, microversion=None):
+        """Deletes share server by its Name or ID."""
+        return self.manila('share-server-delete %s' % share_server,
+                           microversion=microversion)
+
+    def is_share_server_deleted(self, share_server_id, microversion=None):
+        """Says whether share server is deleted or not.
+
+        :param share_server: text -- ID of the share server
+        """
+        servers = self.list_share_servers(microversion=microversion)
+        for list_element in servers:
+            if share_server_id == list_element['Id']:
+                return False
+        return True
+
+    def wait_for_share_server_deletion(self, share_server, microversion=None):
+        """Wait for share server deletion by its Name or ID.
+
+        :param share_server: text -- Name or ID of share server
+        """
+        self.wait_for_resource_deletion(
+            SHARE_SERVER, res_id=share_server, interval=3, timeout=60,
+            microversion=microversion)

manilaclient/tests/functional/test_share_servers.py

@@ -14,34 +14,133 @@
 #    under the License.
 
 import ddt
+from tempest.lib.common.utils import data_utils
 from tempest.lib import exceptions
 
+from manilaclient import config
 from manilaclient.tests.functional import base
 
+CONF = config.CONF
+
 
 @ddt.ddt
-class ManilaClientTestShareServersReadOnly(base.BaseTestCase):
+class ShareServersReadOnlyTest(base.BaseTestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        super(ShareServersReadOnlyTest, cls).setUpClass()
+        cls.client = cls.get_admin_client()
 
     def test_share_server_list(self):
-        self.clients['admin'].manila('share-server-list')
+        self.client.list_share_servers()
 
     def test_share_server_list_with_host_param(self):
-        self.clients['admin'].manila('share-server-list', params='--host host')
+        self.client.list_share_servers(filters={'host': 'fake_host'})
 
     def test_share_server_list_with_status_param(self):
-        self.clients['admin'].manila(
-            'share-server-list', params='--status status')
+        self.client.list_share_servers(filters={'status': 'fake_status'})
 
     def test_share_server_list_with_share_network_param(self):
-        self.clients['admin'].manila(
-            'share-server-list', params='--share-network share-network')
+        self.client.list_share_servers(filters={'share_network': 'fake_sn'})
 
     def test_share_server_list_with_project_id_param(self):
-        self.clients['admin'].manila(
-            'share-server-list', params='--project-id project-id')
+        self.client.list_share_servers(
+            filters={'project_id': 'fake_project_id'})
+
+    @ddt.data(
+        'host', 'status', 'project_id', 'share_network',
+        'host,status,project_id,share_network',
+    )
+    def test_share_server_list_with_specified_columns(self, columns):
+        self.client.list_share_servers(columns=columns)
 
     def test_share_server_list_by_user(self):
         self.assertRaises(
-            exceptions.CommandFailed,
-            self.clients['user'].manila,
-            'share-server-list')
+            exceptions.CommandFailed, self.user_client.list_share_servers)
+
+
+@ddt.ddt
+class ShareServersReadWriteBase(base.BaseTestCase):
+
+    protocol = None
+
+    @classmethod
+    def setUpClass(cls):
+        super(ShareServersReadWriteBase, cls).setUpClass()
+        if not CONF.run_share_servers_tests:
+            message = "share-servers tests are disabled."
+            raise cls.skipException(message)
+        if cls.protocol not in CONF.enable_protocols:
+            message = "%s tests are disabled." % cls.protocol
+            raise cls.skipException(message)
+
+        cls.client = cls.get_admin_client()
+        if not cls.client.share_network:
+            message = "Can run only with DHSS=True mode"
+            raise cls.skipException(message)
+
+    def test_get_and_delete_share_server(self):
+        name = data_utils.rand_name('autotest_share_name')
+        description = data_utils.rand_name('autotest_share_description')
+
+        # We create separate share network to be able to delete share server
+        # further knowing that it is not used by any other concurrent test.
+        common_share_network = self.client.get_share_network(
+            self.client.share_network)
+        neutron_net_id = (
+            common_share_network['neutron_net_id']
+            if 'none' not in common_share_network['neutron_net_id'].lower()
+            else None)
+        neutron_subnet_id = (
+            common_share_network['neutron_subnet_id']
+            if 'none' not in common_share_network['neutron_subnet_id'].lower()
+            else None)
+        share_network = self.client.create_share_network(
+            neutron_net_id=neutron_net_id,
+            neutron_subnet_id=neutron_subnet_id,
+        )
+
+        self.share = self.create_share(
+            share_protocol=self.protocol,
+            size=1,
+            name=name,
+            description=description,
+            share_network=share_network['id'],
+            client=self.client,
+        )
+        share_server_id = self.client.get_share(
+            self.share['id'])['share_server_id']
+
+        # Get share server
+        server = self.client.get_share_server(share_server_id)
+        expected_keys = (
+            'id', 'host', 'status', 'created_at', 'updated_at',
+            'share_network_id', 'share_network_name', 'project_id',
+        )
+        for key in expected_keys:
+            self.assertIn(key, server)
+
+        # Delete share
+        self.client.delete_share(self.share['id'])
+        self.client.wait_for_share_deletion(self.share['id'])
+
+        # Delete share server
+        self.client.delete_share_server(share_server_id)
+        self.client.wait_for_share_server_deletion(share_server_id)
+
+
+class ShareServersReadWriteNFSTest(ShareServersReadWriteBase):
+    protocol = 'nfs'
+
+
+class ShareServersReadWriteCIFSTest(ShareServersReadWriteBase):
+    protocol = 'cifs'
+
+
+def load_tests(loader, tests, _):
+    result = []
+    for test_case in tests:
+        if type(test_case._tests[0]) is ShareServersReadWriteBase:
+            continue
+        result.append(test_case)
+    return loader.suiteClass(result)

manilaclient/tests/unit/v2/fakes.py

@@ -169,7 +169,16 @@ class FakeHTTPClient(fakes.FakeHTTPClient):
         share_servers = {
             'share_servers': {
                 'id': 1234,
-                'share_network_id': 'fake_network_id',
+                'share_network_id': 'fake_network_id_1',
+            },
+        }
+        return (200, {}, share_servers)
+
+    def get_share_servers_5678(self, **kw):
+        share_servers = {
+            'share_servers': {
+                'id': 5678,
+                'share_network_id': 'fake_network_id_2',
             },
         }
         return (200, {}, share_servers)
@@ -254,6 +263,8 @@ class FakeHTTPClient(fakes.FakeHTTPClient):
             raise AssertionError("Unexpected action: %s" % action)
         return (resp, {}, _body)
 
+    post_snapshots_5678_action = post_snapshots_1234_action
+
     def post_snapshots_manage(self, body, **kw):
         _body = {'snapshot': {'id': 'fake'}}
         resp = 202
@@ -452,6 +463,9 @@ class FakeHTTPClient(fakes.FakeHTTPClient):
     def delete_share_servers_1234(self, **kwargs):
         return (202, {}, None)
 
+    def delete_share_servers_5678(self, **kwargs):
+        return (202, {}, None)
+
     def delete_security_services_fake_security_service1(self, **kwargs):
         return (202, {}, None)
 

manilaclient/tests/unit/v2/test_shell.py

@@ -29,7 +29,12 @@ from manilaclient import exceptions
 from manilaclient import shell
 from manilaclient.tests.unit import utils as test_utils
 from manilaclient.tests.unit.v2 import fakes
+from manilaclient.v2 import security_services
 from manilaclient.v2 import share_instances
+from manilaclient.v2 import share_networks
+from manilaclient.v2 import share_servers
+from manilaclient.v2 import share_snapshots
+from manilaclient.v2 import share_types
 from manilaclient.v2 import shell as shell_v2
 
 
@@ -1974,93 +1979,125 @@ class ShellTest(test_utils.TestCase):
         expected = {'reset_task_state': {'task_state': param}}
         self.assert_called('POST', '/shares/1234/action', body=expected)
 
-    @ddt.data('fake-security-service1',
-              'fake-security-service1 fake-security-service2')
-    @mock.patch.object(shell_v2, '_find_security_service', mock.Mock())
-    def test_security_service_delete(self, args):
-        args_split = args.split()
-        shell_v2._find_security_service.side_effect = args_split
+    @ddt.data(('fake_security_service1', ),
+              ('fake_security_service1', 'fake_security_service2'))
+    def test_security_service_delete(self, ss_ids):
+        fake_security_services = [
+            security_services.SecurityService('fake', {'id': ss_id}, True)
+            for ss_id in ss_ids
+        ]
+        self.mock_object(
+            shell_v2, '_find_security_service',
+            mock.Mock(side_effect=fake_security_services))
 
-        self.run_command('security-service-delete %s' % args)
+        self.run_command('security-service-delete %s' % ' '.join(ss_ids))
 
-        self.assert_called_anytime(
-            'DELETE', '/security-services/%s' % args_split[0],
-            clear_callstack=False)
-        self.assert_called_anytime(
-            'DELETE', '/security-services/%s' % args_split[-1])
+        shell_v2._find_security_service.assert_has_calls([
+            mock.call(self.shell.cs, ss_id) for ss_id in ss_ids
+        ])
+        for ss in fake_security_services:
+            self.assert_called_anytime(
+                'DELETE', '/security-services/%s' % ss.id,
+                clear_callstack=False)
 
-    @ddt.data('fake-share-network1',
-              'fake-share-network1 fake-share-network2')
-    @mock.patch.object(shell_v2, '_find_share_network', mock.Mock())
-    def test_share_network_delete(self, args):
-        args_split = args.split()
-        shell_v2._find_share_network.side_effect = args_split
+    @ddt.data(('fake_share_network1', ),
+              ('fake_share_network1', 'fake_share_network1'))
+    def test_share_network_delete(self, sn_ids):
+        fake_share_networks = [
+            share_networks.ShareNetwork('fake', {'id': sn_id}, True)
+            for sn_id in sn_ids
+        ]
+        self.mock_object(
+            shell_v2, '_find_share_network',
+            mock.Mock(side_effect=fake_share_networks))
 
-        self.run_command('share-network-delete %s' % args)
+        self.run_command('share-network-delete %s' % ' '.join(sn_ids))
 
-        self.assert_called_anytime(
-            'DELETE', '/share-networks/%s' % args_split[0],
-            clear_callstack=False)
-        self.assert_called_anytime(
-            'DELETE', '/share-networks/%s' % args_split[-1])
+        shell_v2._find_share_network.assert_has_calls([
+            mock.call(self.shell.cs, sn_id) for sn_id in sn_ids
+        ])
+        for sn in fake_share_networks:
+            self.assert_called_anytime(
+                'DELETE', '/share-networks/%s' % sn.id,
+                clear_callstack=False)
 
-    @ddt.data('fake-snapshot1',
-              'fake-snapshot1 fake-snapshot2')
-    @mock.patch.object(shell_v2, '_find_share_snapshot', mock.Mock())
-    def test_snapshot_delete(self, args):
-        args_split = args.split()
-        shell_v2._find_share_snapshot.side_effect = args_split
+    @ddt.data(('fake_snapshot1', ), ('fake_snapshot1', 'fake_snapshot2'))
+    def test_snapshot_delete(self, snapshot_ids):
+        fake_snapshots = [
+            share_snapshots.ShareSnapshot('fake', {'id': snapshot_id}, True)
+            for snapshot_id in snapshot_ids
+        ]
+        self.mock_object(
+            shell_v2, '_find_share_snapshot',
+            mock.Mock(side_effect=fake_snapshots))
 
-        self.run_command('snapshot-delete %s' % args)
+        self.run_command('snapshot-delete %s' % ' '.join(snapshot_ids))
 
-        self.assert_called_anytime(
-            'DELETE', '/snapshots/%s' % args_split[0],
-            clear_callstack=False)
-        self.assert_called_anytime(
-            'DELETE', '/snapshots/%s' % args_split[-1])
+        shell_v2._find_share_snapshot.assert_has_calls([
+            mock.call(self.shell.cs, s_id) for s_id in snapshot_ids
+        ])
+        for snapshot in fake_snapshots:
+            self.assert_called_anytime(
+                'DELETE', '/snapshots/%s' % snapshot.id,
+                clear_callstack=False)
 
-    @ddt.data('fake-snapshot-force1',
-              'fake-snapshot-force1 fake-snapshot-force2')
-    @mock.patch.object(shell_v2, '_find_share_snapshot', mock.Mock())
-    def test_snapshot_delete_force(self, args):
-        args_split = args.split()
-        shell_v2._find_share_snapshot.side_effect = args_split
+    @ddt.data(('1234', ), ('1234', '5678'))
+    def test_snapshot_force_delete(self, snapshot_ids):
+        fake_snapshots = [
+            share_snapshots.ShareSnapshot('fake', {'id': snapshot_id}, True)
+            for snapshot_id in snapshot_ids
+        ]
+        self.mock_object(
+            shell_v2, '_find_share_snapshot',
+            mock.Mock(side_effect=fake_snapshots))
 
-        self.run_command('snapshot-force-delete %s' % args)
+        self.run_command('snapshot-force-delete %s' % ' '.join(snapshot_ids))
 
-        self.assert_called_anytime(
-            'POST', '/snapshots/%s/action' % args_split[0],
-            {'force_delete': None}, clear_callstack=False)
-        self.assert_called_anytime(
-            'POST', '/snapshots/%s/action' % args_split[-1],
-            {'force_delete': None})
+        shell_v2._find_share_snapshot.assert_has_calls([
+            mock.call(self.shell.cs, s_id) for s_id in snapshot_ids
+        ])
+        for snapshot in fake_snapshots:
+            self.assert_called_anytime(
+                'POST', '/snapshots/%s/action' % snapshot.id,
+                {'force_delete': None},
+                clear_callstack=False)
 
-    @ddt.data('fake-type1',
-              'fake-type1 fake-type2')
-    @mock.patch.object(shell_v2, '_find_share_type', mock.Mock())
-    def test_share_type_delete(self, args):
-        args_split = args.split()
-        shell_v2._find_share_type.side_effect = args_split
+    @ddt.data(('fake_type1', ), ('fake_type1', 'fake_type2'))
+    def test_share_type_delete(self, type_ids):
+        fake_share_types = [
+            share_types.ShareType('fake', {'id': type_id}, True)
+            for type_id in type_ids
+        ]
+        self.mock_object(
+            shell_v2, '_find_share_type',
+            mock.Mock(side_effect=fake_share_types))
 
-        self.run_command('type-delete %s' % args)
+        self.run_command('type-delete %s' % ' '.join(type_ids))
 
-        self.assert_called_anytime(
-            'DELETE', '/types/%s' % args_split[0],
-            clear_callstack=False)
-        self.assert_called_anytime(
-            'DELETE', '/types/%s' % args_split[-1])
+        shell_v2._find_share_type.assert_has_calls([
+            mock.call(self.shell.cs, t_id) for t_id in type_ids
+        ])
+        for fake_share_type in fake_share_types:
+            self.assert_called_anytime(
+                'DELETE', '/types/%s' % fake_share_type.id,
+                clear_callstack=False)
 
-    @ddt.data('fake-share-server1',
-              'fake-share-server1 fake-share-server2')
-    @mock.patch.object(shell_v2, '_find_share_server', mock.Mock())
-    def test_share_server_delete(self, args):
-        args_split = args.split()
-        shell_v2._find_share_server.side_effect = args_split
+    @ddt.data(('1234', ), ('1234', '5678'))
+    def test_share_server_delete(self, server_ids):
+        fake_share_servers = [
+            share_servers.ShareServer('fake', {'id': server_id}, True)
+            for server_id in server_ids
+        ]
+        self.mock_object(
+            shell_v2, '_find_share_server',
+            mock.Mock(side_effect=fake_share_servers))
 
-        self.run_command('share-server-delete %s' % args)
+        self.run_command('share-server-delete %s' % ' '.join(server_ids))
 
-        self.assert_called_anytime(
-            'DELETE', '/share-servers/%s' % args_split[0],
-            clear_callstack=False)
-        self.assert_called_anytime(
-            'DELETE', '/share-servers/%s' % args_split[-1])
+        shell_v2._find_share_server.assert_has_calls([
+            mock.call(self.shell.cs, s_id) for s_id in server_ids
+        ])
+        for server in fake_share_servers:
+            self.assert_called_anytime(
+                'DELETE', '/share-servers/%s' % server.id,
+                clear_callstack=False)

manilaclient/v2/share_servers.py

@@ -38,17 +38,22 @@ class ShareServer(common_base.Resource):
             attr = 'share_network_name'
         return super(ShareServer, self).__getattr__(attr)
 
+    def delete(self):
+        """Delete this share server."""
+        self.manager.delete(self)
+
 
 class ShareServerManager(base.ManagerWithFind):
     """Manage :class:`ShareServer` resources."""
     resource_class = ShareServer
 
-    def get(self, server_id):
+    def get(self, server):
         """Get a share server.
 
-        :param server_id: The ID of the share server to get.
+        :param server: ID of the :class:`ShareServer` to get.
         :rtype: :class:`ShareServer`
         """
+        server_id = common_base.getid(server)
         server = self._get("%s/%s" % (RESOURCES_PATH, server_id),
                            RESOURCE_NAME)
         # Split big dict 'backend_details' to separated strings
@@ -62,20 +67,22 @@ class ShareServerManager(base.ManagerWithFind):
             server._info["details:%s" % k] = v
         return server
 
-    def details(self, server_id):
+    def details(self, server):
         """Get a share server details.
 
-        :param server_id: The ID of the share server to get details from.
+        :param server: ID of the :class:`ShareServer` to get details from.
         :rtype: list of :class:`ShareServerBackendDetails
         """
+        server_id = common_base.getid(server)
         return self._get("%s/%s/details" % (RESOURCES_PATH, server_id),
                          "details")
 
-    def delete(self, server_id):
+    def delete(self, server):
         """Delete share server.
 
-        :param server_id: id of share server to be deleted.
+        :param server: ID of the :class:`ShareServer` to delete.
         """
+        server_id = common_base.getid(server)
         self._delete(RESOURCE_PATH % server_id)
 
     def list(self, search_opts=None):

manilaclient/v2/shell.py

@@ -2605,15 +2605,14 @@ def do_share_server_delete(cs, args):
 
     failure_count = 0
 
-    for id in args.id:
+    for server_id in args.id:
         try:
-            id_ref = _find_share_server(
-                cs, id)
+            id_ref = _find_share_server(cs, server_id)
             cs.share_servers.delete(id_ref)
         except Exception as e:
             failure_count += 1
             print("Delete for share server %s failed: %s" % (
-                id, e), file=sys.stderr)
+                server_id, e), file=sys.stderr)
 
     if failure_count == len(args.id):
         raise exceptions.CommandError("Unable to delete any of the specified "
@@ -2950,15 +2949,14 @@ def do_type_delete(cs, args):
 
     failure_count = 0
 
-    for id in args.id:
+    for name_or_id in args.id:
         try:
-            id_ref = _find_share_type(
-                cs, id)
+            id_ref = _find_share_type(cs, name_or_id)
             cs.share_types.delete(id_ref)
         except Exception as e:
             failure_count += 1
             print("Delete for share type %s failed: %s" % (
-                id, e), file=sys.stderr)
+                name_or_id, e), file=sys.stderr)
 
     if failure_count == len(args.id):
         raise exceptions.CommandError("Unable to delete any of the specified "