validate_args() in rally.cli.cliutils not working correctly if more than 1 required_args in function being validated

Bug #1555764 reported by Piyush on 2016-03-10
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Rally
Undecided
Piyush

Bug Description

<UPDATED AFTER FEEDBACK FROM ALEXANDER>
The validate_args() function in rally.cli.cliutils (https://github.com/openstack/rally/blob/master/rally/cli/cliutils.py#L69) is meant to cross check whether all the required args ( which DO NOT HAVE a default value associated ) are present or not and then invoke the function.

For e.g. rally task start will refer to https://github.com/openstack/rally/blob/master/rally/cli/commands/task.py#L203. In this there is one required argument - task. Thus user HAS TO ALWAYS pass the value for "task" argument while invoking "rally task start"

The current logic for validate_args() is based on count of required_args and NOT the name of the required_args.
This logic may fail if there is more than 1 required args for any function.

For e.g.
>>> from rally.cli.cliutils import validate_args
>>> validate_args(lambda x,y:None,1,x=1)
>>>
lambda x,y:None has 2 required args- x and y
The above case should have thrown an error saying Missing arguments: y. But with current code base it does NOT throw an error for missing argument "y"

NOTE- This issue does not occur so far since all the CLI command functions in various classes ( task, deployment, show, verify, .. ) had only 1 REQUIRED ARGUMENT always. Hence the count logic works currently.

If we introduce / modify any Rally command function in future that needs more than 1 required arguments then the bug condition will appear.

Piyush (pirsriva) on 2016-03-11
Changed in rally:
assignee: nobody → Piyush Raman Srivastava (pirsriva)

Fix proposed to branch: master
Review: https://review.openstack.org/293569

Changed in rally:
status: New → In Progress
Alexander Maretskiy (maretskiy) wrote :

Given paste (http://paste.openstack.org/show/490047/) is not clear, it overcomplicates demonstration with a lot of extra code,
but you can simply run interactive python, import validate_args and give a simple example of wrong validation.

Here is my example, I've not found issues:
http://paste.openstack.org/show/491132/

Piyush (pirsriva) wrote :

@Alexander- Thanks for the feedback.
Kindly refer to https://review.openstack.org/#/c/293569/3/tests/unit/cli/test_cliutils.py and consider the new test case that I have added which covers the case discussed in the bug.

>>> from rally.cli.cliutils import validate_args
>>> validate_args(lambda x,y:None,1,x=1)
>>>
lambda x,y:None has 2 required args- x and y
The above case should have thrown an error saying Missing arguments: y. But with current code base it does NOT throw an error for missing argument "y"

Piyush (pirsriva) wrote :

@Alexander- I'm sorry i did not see your example paste earlier. (http://paste.openstack.org/show/491132/).

Just to add further, in Rally currently all function classes defined in rally.cli.commands.* have only 1 required argument ( e.g. start function in Task class in rally.cli.commands..task ) and hence the logic of count works great so far!!

 However, in case we make > 1 required args in any function in future, then the bug will reproduce. I thought it would be better to harden the validate_args() function and NOT work under the assumption of having only 1 required argument in all functions always.

description: updated
Alexander Maretskiy (maretskiy) wrote :

Thank you for clear explanation, now it is obvious that validate_args() requires improvements!

Reviewed: https://review.openstack.org/293569
Committed: https://git.openstack.org/cgit/openstack/rally/commit/?id=7dac478026ec69ca70f36485408fd8d2afa94a2d
Submitter: Jenkins
Branch: master

commit 7dac478026ec69ca70f36485408fd8d2afa94a2d
Author: Piyush Raman Srivastava <email address hidden>
Date: Wed Mar 16 09:46:59 2016 -0700

    Fix validate_args function in cliutils

    Fix validate_args() for more strict validation. Current
    logic works if only 1 required arg for functions.
    May fails if more than 1 required args.
    Refer to bug description for example failure case.

    Change-Id: Ie9bc54631d431f0933daf74793f0bcca1ba227f7
    Closes-Bug: #1555764

Changed in rally:
status: In Progress → Fix Released

This issue was fixed in the openstack/rally 0.4.0 release.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers