From fabec4fafca9003db8f56a8d99307f119e665d5c Mon Sep 17 00:00:00 2001 From: Sharat Sharma Date: Fri, 7 Jul 2017 13:50:37 +0530 Subject: [PATCH] Set the default value of --limit parameter The "--limit" parameter of heavy CLI commands should not be infinite by default. For commands like "mistral task-list" and "execution-list" we need to set --limit parameter by default to some reasonable value. The proposal is 100. Otherwise it's easy to cause a huge load on a server if the result set is not limited. A warning message is thrown whenever these commands are executed without using "--limit" parameter. Change-Id: If0fead181171e5b9c590f3f4c852c6ad2541ff94 Implements: blueprint mistral-cli-default-limit-parameter --- mistralclient/api/v2/action_executions.py | 7 ++++- .../commands/v2/action_executions.py | 17 ++++++++++- mistralclient/commands/v2/base.py | 3 ++ mistralclient/commands/v2/executions.py | 10 ++++++- mistralclient/commands/v2/tasks.py | 15 ++++++++++ .../tests/functional/cli/v2/cli_tests_v2.py | 30 +++++++++++++++++++ .../tests/unit/v2/test_cli_executions.py | 2 +- ...-default-limit-value-7e293d843d6d85ac.yaml | 7 +++++ 8 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/set-default-limit-value-7e293d843d6d85ac.yaml diff --git a/mistralclient/api/v2/action_executions.py b/mistralclient/api/v2/action_executions.py index 90934984..8dd4c77f 100644 --- a/mistralclient/api/v2/action_executions.py +++ b/mistralclient/api/v2/action_executions.py @@ -63,12 +63,17 @@ class ActionExecutionManager(base.ResourceManager): return self._update('/action_executions/%s' % id, data) - def list(self, task_execution_id=None): + def list(self, task_execution_id=None, limit=None): url = '/action_executions' + qparams = {} + if task_execution_id: url = '/tasks/%s/action_executions' % task_execution_id + if limit: + qparams['limit'] = limit + return self._list(url, response_key='action_executions') def get(self, id): diff --git a/mistralclient/commands/v2/action_executions.py b/mistralclient/commands/v2/action_executions.py index cdef79d5..c96efb06 100644 --- a/mistralclient/commands/v2/action_executions.py +++ b/mistralclient/commands/v2/action_executions.py @@ -182,14 +182,29 @@ class List(base.MistralLister): nargs='?', help='Task execution ID.' ) + parser.add_argument( + '--limit', + type=int, + help='Maximum number of action-executions to return in a single ' + 'result. limit is set to %s by default. Use --limit -1 to ' + 'fetch the full result set.' % base.DEFAULT_LIMIT, + nargs='?' + ) return parser def _get_resources(self, parsed_args): + if parsed_args.limit is None: + parsed_args.limit = base.DEFAULT_LIMIT + LOG.info("limit is set to %s by default. Set " + "the limit explicitly using \'--limit\', if required. " + "Use \'--limit\' -1 to fetch the full result set.", + base.DEFAULT_LIMIT) mistral_client = self.app.client_manager.workflow_engine return mistral_client.action_executions.list( - parsed_args.task_execution_id + parsed_args.task_execution_id, + limit=parsed_args.limit, ) diff --git a/mistralclient/commands/v2/base.py b/mistralclient/commands/v2/base.py index a7130f21..cbfe6da8 100644 --- a/mistralclient/commands/v2/base.py +++ b/mistralclient/commands/v2/base.py @@ -21,6 +21,9 @@ from osc_lib.command import command import six +DEFAULT_LIMIT = 100 + + @six.add_metaclass(abc.ABCMeta) class MistralLister(command.Lister): @abc.abstractmethod diff --git a/mistralclient/commands/v2/executions.py b/mistralclient/commands/v2/executions.py index 67490bc3..a3c85680 100644 --- a/mistralclient/commands/v2/executions.py +++ b/mistralclient/commands/v2/executions.py @@ -93,7 +93,9 @@ class List(base.MistralLister): parser.add_argument( '--limit', type=int, - help='Maximum number of executions to return in a single result.', + help='Maximum number of executions to return in a single result. ' + 'limit is set to %s by default. Use --limit -1 to fetch the ' + 'full result set.' % base.DEFAULT_LIMIT, nargs='?' ) parser.add_argument( @@ -122,6 +124,12 @@ class List(base.MistralLister): return parser def _get_resources(self, parsed_args): + if parsed_args.limit is None: + parsed_args.limit = base.DEFAULT_LIMIT + LOG.info("limit is set to %s by default. Set " + "the limit explicitly using \'--limit\', if required. " + "Use \'--limit\' -1 to fetch the full result set.", + base.DEFAULT_LIMIT) mistral_client = self.app.client_manager.workflow_engine return mistral_client.executions.list( diff --git a/mistralclient/commands/v2/tasks.py b/mistralclient/commands/v2/tasks.py index 6fac5aaf..396d3e82 100644 --- a/mistralclient/commands/v2/tasks.py +++ b/mistralclient/commands/v2/tasks.py @@ -80,6 +80,14 @@ class List(base.MistralLister): action='append', help='Filters. Can be repeated.' ) + parser.add_argument( + '--limit', + type=int, + help='Maximum number of tasks to return in a single result. ' + 'limit is set to %s by default. Use --limit -1 to fetch the ' + 'full result set.' % base.DEFAULT_LIMIT, + nargs='?' + ) return parser @@ -87,10 +95,17 @@ class List(base.MistralLister): return format_list def _get_resources(self, parsed_args): + if parsed_args.limit is None: + parsed_args.limit = base.DEFAULT_LIMIT + LOG.info("limit is set to %s by default. Set " + "the limit explicitly using \'--limit\', if required. " + "Use \'--limit\' -1 to fetch the full result set.", + base.DEFAULT_LIMIT) mistral_client = self.app.client_manager.workflow_engine return mistral_client.tasks.list( parsed_args.workflow_execution, + limit=parsed_args.limit, **base.get_filters(parsed_args) ) diff --git a/mistralclient/tests/functional/cli/v2/cli_tests_v2.py b/mistralclient/tests/functional/cli/v2/cli_tests_v2.py index 999ca2ae..235e1688 100644 --- a/mistralclient/tests/functional/cli/v2/cli_tests_v2.py +++ b/mistralclient/tests/functional/cli/v2/cli_tests_v2.py @@ -97,6 +97,16 @@ class SimpleMistralCLITests(base.MistralCLIAuth): ['ID', 'Name', 'Workflow name', 'State', 'Accepted'] ) + def test_action_execution_list_with_limit(self): + act_execs = self.parser.listing( + self.mistral( + 'action-execution-list', + params='--limit 1' + ) + ) + + self.assertEqual(1, len(act_execs)) + class WorkbookCLITests(base_v2.MistralClientTestBase): """Test suite checks commands to work with workbooks.""" @@ -853,6 +863,26 @@ class TaskCLITests(base_v2.MistralClientTestBase): self.assertEqual('goodbye', tasks[0]['Name']) + def test_task_list_with_limit(self): + wf_exec = self.execution_create( + "%s input task_name" % self.reverse_wf['Name'] + ) + + exec_id = self.get_field_value(wf_exec, 'ID') + + self.assertTrue(self.wait_execution_success(exec_id)) + + tasks = self.parser.listing(self.mistral('task-list')) + + tasks = self.parser.listing( + self.mistral( + 'task-list', + params='--limit 1' + ) + ) + + self.assertEqual(1, len(tasks)) + class ActionCLITests(base_v2.MistralClientTestBase): """Test suite checks commands to work with actions.""" diff --git a/mistralclient/tests/unit/v2/test_cli_executions.py b/mistralclient/tests/unit/v2/test_cli_executions.py index adaf1a9c..f65368dd 100644 --- a/mistralclient/tests/unit/v2/test_cli_executions.py +++ b/mistralclient/tests/unit/v2/test_cli_executions.py @@ -229,7 +229,7 @@ class TestCLIExecutionsV2(base.BaseCommandTest): self.call(execution_cmd.List) self.client.executions.list.assert_called_once_with( - limit=None, + limit=100, marker='', sort_dirs='asc', sort_keys='created_at', diff --git a/releasenotes/notes/set-default-limit-value-7e293d843d6d85ac.yaml b/releasenotes/notes/set-default-limit-value-7e293d843d6d85ac.yaml new file mode 100644 index 00000000..08cb1d3b --- /dev/null +++ b/releasenotes/notes/set-default-limit-value-7e293d843d6d85ac.yaml @@ -0,0 +1,7 @@ +--- +critical: + - | + The "--limit" parameter of heavy CLI commands like "task-list" and + "execution-list" and "action-execution-list" is set to "100" by + default to avoid the huge load on server. To fetch the full result + set, user may use "--limit -1".