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

Bug #1555764 reported by Piyush
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Rally
Fix Released
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)
Changed in rally:
assignee: nobody → Piyush Raman Srivastava (pirsriva)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to rally (master)

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

Changed in rally:
status: New → In Progress
Revision history for this message
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/

Revision history for this message
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"

Revision history for this message
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
Revision history for this message
Alexander Maretskiy (maretskiy) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to rally (master)

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
Revision history for this message
Thierry Carrez (ttx) wrote : Fix included in openstack/rally 0.4.0

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  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.