From c7456c675f21862421c2e8738f608d18f0045f0b Mon Sep 17 00:00:00 2001 From: Your Name Date: Tue, 30 Sep 2014 04:34:07 -0400 Subject: [PATCH] Improve share list API filtering 1) Added new params for filtering by: - share network - snapshot - volume type - host - limit and offset a la pagination - project id (useful with '--all-tenants') - metadata* - extra-specs* - direction (asc, desc)* - key of share* * requires server-side update 2) Added aliases for params, mostly in perspective to avoid mess of underscore and dash symbols. 3) Updated utils.print_list func to print results as is without reordering. Implements blueprint improve-share-list-filtering Change-Id: I6fe92c5242f18c900e109271f9182bd5ed287b25 --- manilaclient/common/__init__.py | 0 manilaclient/common/constants.py | 29 ++++++ manilaclient/utils.py | 13 +-- manilaclient/v1/shares.py | 32 +++++- manilaclient/v1/shell.py | 158 +++++++++++++++++++++++++--- tests/test_shell.py | 2 +- tests/v1/fakes.py | 39 ++++++- tests/v1/test_shares.py | 51 ++++++++- tests/v1/test_shell.py | 174 +++++++++++++++++++++++++++++-- 9 files changed, 460 insertions(+), 38 deletions(-) create mode 100644 manilaclient/common/__init__.py create mode 100644 manilaclient/common/constants.py diff --git a/manilaclient/common/__init__.py b/manilaclient/common/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/manilaclient/common/constants.py b/manilaclient/common/constants.py new file mode 100644 index 000000000..9bb1d7bda --- /dev/null +++ b/manilaclient/common/constants.py @@ -0,0 +1,29 @@ +# Copyright 2014 Mirantis, Inc. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""Common constants that can be used all over the manilaclient.""" + +# These are used for providing desired sorting params with list requests +SORT_DIR_VALUES = ('asc', 'desc') +SHARE_SORT_KEY_VALUES = ( + 'id', 'status', 'size', 'host', 'share_proto', + 'export_location', 'availability_zone', + 'user_id', 'project_id', + 'created_at', 'updated_at', + 'display_name', 'name', + 'volume_type_id', 'volume_type', + 'share_network_id', 'share_network', + 'snapshot_id', 'snapshot', +) diff --git a/manilaclient/utils.py b/manilaclient/utils.py index b7f1e5921..07c4b0f42 100644 --- a/manilaclient/utils.py +++ b/manilaclient/utils.py @@ -20,15 +20,14 @@ import six from manilaclient.openstack.common import strutils -def _print(pt, order): +def _print(pt, order=None): + output = pt.get_string(sortby=order) if order else pt.get_string() if sys.version_info >= (3, 0): - print(pt.get_string(sortby=order)) + print(output) else: - print(strutils.safe_encode(pt.get_string(sortby=order))) + print(strutils.safe_encode(output)) -# NOTE(vponomaryov): replace function 'print_list' and 'print_dict' -# with functions from cliutils, when bug #1342050 is fixed def print_list(objs, fields, formatters={}, order_by=None): mixed_case_fields = ['serverId'] pt = prettytable.PrettyTable([f for f in fields], caching=False) @@ -47,10 +46,6 @@ def print_list(objs, fields, formatters={}, order_by=None): data = getattr(o, field_name, '') row.append(data) pt.add_row(row) - - if order_by is None: - order_by = fields[0] - _print(pt, order_by) diff --git a/manilaclient/v1/shares.py b/manilaclient/v1/shares.py index fe3e61f2f..64a69a8b5 100644 --- a/manilaclient/v1/shares.py +++ b/manilaclient/v1/shares.py @@ -22,6 +22,7 @@ except ImportError: from urllib.parse import urlencode # noqa from manilaclient import base +from manilaclient.common import constants from manilaclient import exceptions from manilaclient.openstack.common.apiclient import base as common_base @@ -175,11 +176,40 @@ class ShareManager(base.ManagerWithFind): body = {'share': kwargs, } return self._update("/shares/%s" % share.id, body) - def list(self, detailed=True, search_opts=None): + def list(self, detailed=True, search_opts=None, + sort_key=None, sort_dir=None): """Get a list of all shares. + :param detailed: Whether to return detailed share info. + :param search_opts: Search options to filter out shares. + :param sort_key: Key to be sorted. + :param sort_dir: Sort direction, should be 'desc' or 'asc'. :rtype: list of :class:`Share` """ + if search_opts is None: + search_opts = {} + + if sort_key is not None: + if sort_key in constants.SHARE_SORT_KEY_VALUES: + search_opts['sort_key'] = sort_key + # NOTE(vponomaryov): Replace aliases with appropriate keys + if sort_key == 'volume_type': + search_opts['sort_key'] = 'volume_type_id' + elif sort_key == 'snapshot': + search_opts['sort_key'] = 'snapshot_id' + elif sort_key == 'share_network': + search_opts['sort_key'] = 'share_network_id' + else: + raise ValueError('sort_key must be one of the following: %s.' + % ', '.join(constants.SHARE_SORT_KEY_VALUES)) + + if sort_dir is not None: + if sort_dir in constants.SORT_DIR_VALUES: + search_opts['sort_dir'] = sort_dir + else: + raise ValueError('sort_dir must be one of the following: %s.' + % ', '.join(constants.SORT_DIR_VALUES)) + if search_opts: query_string = urlencode( sorted([(k, v) for (k, v) in list(search_opts.items()) if v])) diff --git a/manilaclient/v1/shell.py b/manilaclient/v1/shell.py index 54e5a2d30..d0f208bd5 100644 --- a/manilaclient/v1/shell.py +++ b/manilaclient/v1/shell.py @@ -19,6 +19,7 @@ import os import sys import time +from manilaclient.common import constants from manilaclient import exceptions from manilaclient.openstack.common import cliutils from manilaclient import utils @@ -96,18 +97,34 @@ def _translate_keys(collection, convert): def _extract_metadata(args): metadata = {} - for metadatum in args.metadata: - # unset doesn't require a val, so we have the if/else - if '=' in metadatum: - (key, value) = metadatum.split('=', 1) - else: - key = metadatum - value = None + if args.metadata: + for metadatum in args.metadata: + # unset doesn't require a val, so we have the if/else + if '=' in metadatum: + (key, value) = metadatum.split('=', 1) + else: + key = metadatum + value = None - metadata[key] = value + metadata[key] = value return metadata +def _extract_extra_specs(args): + extra_specs = {} + if args.extra_specs: + for extra_spec in args.extra_specs: + # unset doesn't require a val, so we have the if/else + if '=' in extra_spec: + (key, value) = extra_spec.split('=', 1) + else: + key = extra_spec + value = None + + extra_specs[key] = value + return extra_specs + + def do_endpoints(cs, args): """Discover endpoints that get returned from the authenticate services.""" catalog = cs.client.service_catalog.catalog @@ -545,32 +562,147 @@ def do_access_list(cs, args): @cliutils.arg( '--name', metavar='', + type=str, default=None, help='Filter results by name.') @cliutils.arg( '--status', metavar='', + type=str, default=None, help='Filter results by status.') @cliutils.arg( '--share-server-id', + '--share-server_id', '--share_server-id', '--share_server_id', # aliases metavar='', + type=str, default=None, + action='single_alias', help='Filter results by share server ID.') +@cliutils.arg( + '--metadata', + type=str, + nargs='*', + metavar='', + help='Filters results by a metadata key and value. OPTIONAL: Default=None', + default=None) +@cliutils.arg( + '--extra-specs', + '--extra_specs', # alias + type=str, + nargs='*', + metavar='', + action='single_alias', + help='Filters results by a extra specs key and value of volume type that ' + 'was used for share creation. OPTIONAL: Default=None', + default=None) +@cliutils.arg( + '--volume-type', + '--volume_type', '--volume-type-id', # aliases + '--volume-type_id', '--volume_type-id', '--volume_type_id', # aliases + metavar='', + type=str, + default=None, + action='single_alias', + help='Filter results by a volume type id or name that was used for share ' + 'creation.') +@cliutils.arg( + '--limit', + metavar='', + type=int, + default=None, + help='Maximum number of shares to return. OPTIONAL: Default=None.') +@cliutils.arg( + '--offset', + metavar='', + type=int, + default=None, + help='Set offset to define start point of share listing. ' + 'OPTIONAL: Default=None.') +@cliutils.arg( + '--sort-key', + '--sort_key', # alias + metavar='', + type=str, + default=None, + action='single_alias', + help='Key to be sorted, available keys are %(keys)s. ' + 'OPTIONAL: Default=None.' % {'keys': constants.SHARE_SORT_KEY_VALUES}) +@cliutils.arg( + '--sort-dir', + '--sort_dir', # alias + metavar='', + type=str, + default=None, + action='single_alias', + help='Sort direction, available values are %(values)s. ' + 'OPTIONAL: Default=None.' % {'values': constants.SORT_DIR_VALUES}) +@cliutils.arg( + '--snapshot', + metavar='', + type=str, + default=None, + help='Filer results by snapshot name or id, that was used for share.') +@cliutils.arg( + '--host', + metavar='', + default=None, + help='Filter results by host.') +@cliutils.arg( + '--share-network', + '--share_network', # alias + metavar='', + type=str, + default=None, + action='single_alias', + help='Filter results by share-network name or id.') +@cliutils.arg( + '--project-id', + '--project_id', # alias + metavar='', + type=str, + default=None, + action='single_alias', + help="Filter results by project id. Useful with set key '--all-tenants'.") @cliutils.service_type('share') def do_list(cs, args): - """List all NAS shares.""" + """List NAS shares with filters.""" + list_of_keys = [ + 'ID', 'Name', 'Size', 'Share Proto', 'Status', 'Volume Type', + 'Export location', 'Host', + ] all_tenants = int(os.environ.get("ALL_TENANTS", args.all_tenants)) + + empty_obj = type('Empty', (object,), {'id': None}) + volume_type = (_find_volume_type(cs, args.volume_type) + if args.volume_type else empty_obj) + + snapshot = (_find_share_snapshot(cs, args.snapshot) + if args.snapshot else empty_obj) + + share_network = (_find_share_network(cs, args.share_network) + if args.share_network else empty_obj) search_opts = { + 'offset': args.offset, + 'limit': args.limit, 'all_tenants': all_tenants, 'name': args.name, 'status': args.status, + 'host': args.host, + 'share_network_id': share_network.id, + 'snapshot_id': snapshot.id, + 'volume_type_id': volume_type.id, + 'metadata': _extract_metadata(args), + 'extra_specs': _extract_extra_specs(args), 'share_server_id': args.share_server_id, + 'project_id': args.project_id, } - shares = cs.shares.list(search_opts=search_opts) - utils.print_list(shares, - ['ID', 'Name', 'Size', 'Share Proto', 'Status', - 'Export location']) + shares = cs.shares.list( + search_opts=search_opts, + sort_key=args.sort_key, + sort_dir=args.sort_dir, + ) + utils.print_list(shares, list_of_keys) @cliutils.arg( diff --git a/tests/test_shell.py b/tests/test_shell.py index efe7d1f26..c50b3fb7a 100644 --- a/tests/test_shell.py +++ b/tests/test_shell.py @@ -73,7 +73,7 @@ class ShellTest(utils.TestCase): def test_help_on_subcommand(self): required = [ '.*?^usage: manila list', - '.*?(?m)^List all NAS shares.', + '.*?(?m)^List NAS shares with filters.', ] help_text = self.shell('help list') for r in required: diff --git a/tests/v1/fakes.py b/tests/v1/fakes.py index 12bfbeadb..5f0134cdb 100644 --- a/tests/v1/fakes.py +++ b/tests/v1/fakes.py @@ -34,10 +34,43 @@ class FakeHTTPClient(fakes.FakeHTTPClient): share = {'share': {'id': 1234, 'name': 'sharename'}} return (200, {}, share) + def get_shares(self, **kw): + endpoint = "http://127.0.0.1:8786/v1" + share_id = '1234' + shares = { + 'shares': [ + { + 'id': share_id, + 'name': 'sharename', + 'links': [ + {"href": endpoint + "/fake_project/shares/" + share_id, + "rel": "self"}, + ], + }, + ] + } + return (200, {}, shares) + def get_shares_detail(self, **kw): - shares = {'shares': [{'id': 1234, - 'name': 'sharename', - 'attachments': [{'server_id': 111}]}]} + endpoint = "http://127.0.0.1:8786/v1" + share_id = '1234' + shares = { + 'shares': [ + { + 'id': share_id, + 'name': 'sharename', + 'status': 'fake_status', + 'size': 1, + 'host': 'fake_host', + 'export_location': 'fake_export_location', + 'snapshot_id': 'fake_snapshot_id', + 'links': [ + {"href": endpoint + "/fake_project/shares/" + share_id, + "rel": "self"}, + ], + }, + ] + } return (200, {}, shares) def get_snapshots_1234(self, **kw): diff --git a/tests/v1/test_shares.py b/tests/v1/test_shares.py index 90947be63..4f4ec251c 100644 --- a/tests/v1/test_shares.py +++ b/tests/v1/test_shares.py @@ -90,10 +90,57 @@ class SharesTest(utils.TestCase): cs.assert_called('POST', '/shares/1234/action', {'os-force_delete': None}) - def test_list_shares(self): - cs.shares.list() + def test_list_shares_index(self): + cs.shares.list(detailed=False) + cs.assert_called('GET', '/shares') + + def test_list_shares_index_with_search_opts(self): + search_opts = { + 'fake_str': 'fake_str_value', + 'fake_int': 1, + } + cs.shares.list(detailed=False, search_opts=search_opts) + cs.assert_called('GET', '/shares?fake_int=1&fake_str=fake_str_value') + + def test_list_shares_detailed(self): + cs.shares.list(detailed=True) cs.assert_called('GET', '/shares/detail') + def test_list_shares_detailed_with_search_opts(self): + search_opts = { + 'fake_str': 'fake_str_value', + 'fake_int': 1, + } + cs.shares.list(detailed=True, search_opts=search_opts) + cs.assert_called( + 'GET', '/shares/detail?fake_int=1&fake_str=fake_str_value') + + def test_list_shares_sort_by_asc_and_host_key(self): + cs.shares.list(detailed=False, sort_key='host', sort_dir='asc') + cs.assert_called('GET', '/shares?sort_dir=asc&sort_key=host') + + def test_list_shares_sort_by_desc_and_size_key(self): + cs.shares.list(detailed=False, sort_key='size', sort_dir='desc') + cs.assert_called('GET', '/shares?sort_dir=desc&sort_key=size') + + def test_list_shares_filter_by_share_network_alias(self): + cs.shares.list(detailed=False, sort_key='share_network') + cs.assert_called('GET', '/shares?sort_key=share_network_id') + + def test_list_shares_filter_by_snapshot_alias(self): + cs.shares.list(detailed=False, sort_key='snapshot') + cs.assert_called('GET', '/shares?sort_key=snapshot_id') + + def test_list_shares_filter_by_volume_type_alias(self): + cs.shares.list(detailed=False, sort_key='volume_type') + cs.assert_called('GET', '/shares?sort_key=volume_type_id') + + def test_list_shares_by_improper_direction(self): + self.assertRaises(ValueError, cs.shares.list, sort_dir='fake') + + def test_list_shares_by_improper_key(self): + self.assertRaises(ValueError, cs.shares.list, sort_key='fake') + def test_allow_access_to_share(self): share = cs.shares.get(1234) ip = '192.168.0.1' diff --git a/tests/v1/test_shell.py b/tests/v1/test_shell.py index 799715e4a..9736dc3cd 100644 --- a/tests/v1/test_shell.py +++ b/tests/v1/test_shell.py @@ -1,4 +1,5 @@ # Copyright 2013 OpenStack LLC. +# Copyright 2014 Mirantis, Inc. # All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); you may @@ -18,7 +19,9 @@ import mock import requests from manilaclient import client +from manilaclient.common import constants from manilaclient import exceptions +from manilaclient.openstack.common import cliutils from manilaclient.openstack.common import jsonutils from manilaclient import shell from manilaclient.v1 import client as client_v1 @@ -51,6 +54,10 @@ class ShellTest(utils.TestCase): self.old_get_client_class = client.get_client_class client.get_client_class = lambda *_: fakes.FakeClient + # Following shows available separators for optional params + # and its values + self.separators = [' ', '='] + def tearDown(self): # For some method like test_image_meta_bad_action we are # testing a SystemExit to be thrown and object self.shell has @@ -79,20 +86,169 @@ class ShellTest(utils.TestCase): self.assert_called('GET', '/shares/detail') def test_list_filter_status(self): - self.run_command('list --status=available') - self.assert_called('GET', '/shares/detail?status=available') + for separator in self.separators: + self.run_command('list --status' + separator + 'available') + self.assert_called('GET', '/shares/detail?status=available') def test_list_filter_name(self): - self.run_command('list --name=1234') - self.assert_called('GET', '/shares/detail?name=1234') + for separator in self.separators: + self.run_command('list --name' + separator + '1234') + self.assert_called('GET', '/shares/detail?name=1234') - def test_list_all_tenants(self): - self.run_command('list --all-tenants=1') + def test_list_all_tenants_only_key(self): + self.run_command('list --all-tenants') self.assert_called('GET', '/shares/detail?all_tenants=1') - def test_list_filter_share_server_id(self): - self.run_command('list --share-server-id=1234') - self.assert_called('GET', '/shares/detail?share_server_id=1234') + def test_list_all_tenants_key_and_value_1(self): + for separator in self.separators: + self.run_command('list --all-tenants' + separator + '1') + self.assert_called('GET', '/shares/detail?all_tenants=1') + + def test_list_all_tenants_key_and_value_0(self): + for separator in self.separators: + self.run_command('list --all-tenants' + separator + '0') + self.assert_called('GET', '/shares/detail') + + def test_list_filter_by_share_server_and_its_aliases(self): + aliases = [ + '--share-server-id', '--share-server_id', + '--share_server-id', '--share_server_id', + ] + for alias in aliases: + for separator in self.separators: + self.run_command('list ' + alias + separator + '1234') + self.assert_called( + 'GET', '/shares/detail?share_server_id=1234') + + def test_list_filter_by_metadata(self): + self.run_command('list --metadata key=value') + self.assert_called( + 'GET', '/shares/detail?metadata=%7B%27key%27%3A+%27value%27%7D') + + def test_list_filter_by_extra_specs_and_its_aliases(self): + aliases = ['--extra-specs', '--extra_specs', ] + for alias in aliases: + self.run_command('list ' + alias + ' key=value') + self.assert_called( + 'GET', + '/shares/detail?extra_specs=%7B%27key%27%3A+%27value%27%7D', + ) + + def test_list_filter_by_volume_type_and_its_aliases(self): + fake_vt = type('Empty', (object,), {'id': 'fake_vt'}) + aliases = [ + '--volume-type', '--volume_type', '--volume-type-id', + '--volume-type_id', '--volume_type-id', '--volume_type_id', + ] + for alias in aliases: + for separator in self.separators: + with mock.patch.object(cliutils, 'find_resource', + mock.Mock(return_value=fake_vt)): + self.run_command('list ' + alias + separator + fake_vt.id) + self.assert_called( + 'GET', '/shares/detail?volume_type_id=' + fake_vt.id) + + def test_list_filter_by_volume_type_not_found(self): + for separator in self.separators: + self.assertRaises( + exceptions.CommandError, + self.run_command, + 'list --volume-type' + separator + 'not_found_expected', + ) + self.assert_called('GET', '/types') + + def test_list_with_limit(self): + for separator in self.separators: + self.run_command('list --limit' + separator + '50') + self.assert_called('GET', '/shares/detail?limit=50') + + def test_list_with_offset(self): + for separator in self.separators: + self.run_command('list --offset' + separator + '50') + self.assert_called('GET', '/shares/detail?offset=50') + + def test_list_with_sort_dir_verify_keys(self): + # Verify allowed aliases and keys + aliases = ['--sort_dir', '--sort-dir'] + for alias in aliases: + for key in constants.SORT_DIR_VALUES: + for separator in self.separators: + self.run_command('list ' + alias + separator + key) + self.assert_called('GET', '/shares/detail?sort_dir=' + key) + + def test_list_with_fake_sort_dir(self): + self.assertRaises( + ValueError, + self.run_command, + 'list --sort-dir fake_sort_dir', + ) + + def test_list_with_sort_key_verify_keys(self): + # Verify allowed aliases and keys + aliases = ['--sort_key', '--sort-key'] + for alias in aliases: + for key in constants.SHARE_SORT_KEY_VALUES: + for separator in self.separators: + self.run_command('list ' + alias + separator + key) + key = 'share_network_id' if key == 'share_network' else key + key = 'snapshot_id' if key == 'snapshot' else key + key = 'volume_type_id' if key == 'volume_type' else key + self.assert_called('GET', '/shares/detail?sort_key=' + key) + + def test_list_with_fake_sort_key(self): + self.assertRaises( + ValueError, + self.run_command, + 'list --sort-key fake_sort_key', + ) + + def test_list_filter_by_snapshot(self): + fake_s = type('Empty', (object,), {'id': 'fake_snapshot_id'}) + for separator in self.separators: + with mock.patch.object(cliutils, 'find_resource', + mock.Mock(return_value=fake_s)): + self.run_command('list --snapshot' + separator + fake_s.id) + self.assert_called( + 'GET', '/shares/detail?snapshot_id=' + fake_s.id) + + def test_list_filter_by_snapshot_not_found(self): + self.assertRaises( + exceptions.CommandError, + self.run_command, + 'list --snapshot not_found_expected', + ) + self.assert_called('GET', '/snapshots/detail') + + def test_list_filter_by_host(self): + for separator in self.separators: + self.run_command('list --host' + separator + 'fake_host') + self.assert_called('GET', '/shares/detail?host=fake_host') + + def test_list_filter_by_share_network(self): + aliases = ['--share-network', '--share_network', ] + fake_sn = type('Empty', (object,), {'id': 'fake_share_network_id'}) + for alias in aliases: + for separator in self.separators: + with mock.patch.object(cliutils, 'find_resource', + mock.Mock(return_value=fake_sn)): + self.run_command('list ' + alias + separator + fake_sn.id) + self.assert_called( + 'GET', '/shares/detail?share_network_id=' + fake_sn.id) + + def test_list_filter_by_share_network_not_found(self): + self.assertRaises( + exceptions.CommandError, + self.run_command, + 'list --share-network not_found_expected', + ) + self.assert_called('GET', '/share-networks/detail') + + def test_list_filter_by_project_id(self): + aliases = ['--project-id', '--project_id'] + for alias in aliases: + for separator in self.separators: + self.run_command('list ' + alias + separator + 'fake_id') + self.assert_called('GET', '/shares/detail?project_id=fake_id') def test_show(self): self.run_command('show 1234')