Change in vdsm[master]: lvm: Enhance changelv function to fix option repeated error

nsoffer at redhat.com nsoffer at redhat.com
Wed Mar 11 08:49:50 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: lvm: Enhance changelv function to fix option repeated error
......................................................................


Patch Set 6:

(5 comments)

https://gerrit.ovirt.org/#/c/38195/6//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2015-02-26 17:13:42 +0530
Line 4: Commit:     Timothy Asir Jeyasingh <tjeyasin at redhat.com>
Line 5: CommitDate: 2015-03-11 06:54:33 +0530
Line 6: 
Line 7: lvm: Enhance changelv function to fix option repeated error
I think what you mean is:

    Allow autobackup option in changelv()
Line 8: 
Line 9: Currently autobackup option sets to 'false' inside the function in
Line 10: most of the lvm functions like lvcreate, lvremove, etc., in storage.lvm.
Line 11: This will not allow any call to automatically back up metadata after a change.


Line 12: 
Line 13: Also currently most of the functions in lvm module accepts list
Line 14: of params in a tuple format and have some mandatory options.
Line 15: This will allow to break the lvm call if any thing called with
Line 16: different values for such mandatory options.
I don't understand the last sentence. Do you mean: This may cause lvm call to fail if called with multiple conflicting options?
Line 17: 
Line 18: ex:- lvchange(vg, lvs, ('-autobackup', 'y'))
Line 19: This will returns "Option -A/--autobackup may not be repeated" error.
Line 20: 


Line 15: This will allow to break the lvm call if any thing called with
Line 16: different values for such mandatory options.
Line 17: 
Line 18: ex:- lvchange(vg, lvs, ('-autobackup', 'y'))
Line 19: This will returns "Option -A/--autobackup may not be repeated" error.
Lets make it more clear:

    Example: lvchange(vg, lvs, ('-autobackup', 'y'))
    Will fail with "Option -A/--autobackup may not be repeated" error.
Line 20: 
Line 21: This patch fix this issue for changelv function.
Line 22: 
Line 23: Change-Id: I8190878fe071fad46800e666795b94779d745abf


Line 17: 
Line 18: ex:- lvchange(vg, lvs, ('-autobackup', 'y'))
Line 19: This will returns "Option -A/--autobackup may not be repeated" error.
Line 20: 
Line 21: This patch fix this issue for changelv function.
Why do changelv must support -autobackup y, and other functions do not?

How are you going to use this?
Line 22: 
Line 23: Change-Id: I8190878fe071fad46800e666795b94779d745abf


https://gerrit.ovirt.org/#/c/38195/6/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 802:     # so we invalidate cache to reload these volumes on first occasion
Line 803:     lvnames = tuple("%s/%s" % (vg, lv) for lv in lvs)
Line 804:     cmd = ["lvchange"]
Line 805:     if not (('--autobackup', 'n') in attrs or ('--autobackup', 'y') in attrs
Line 806:             or ('-A', 'n') in attrs or ('-A', 'y') in attrs):
This will not work as attrs may be a single tuple or list of tuples. It will work
for your call now, because you will call it with list of tuples, but someone else
will trip into this later when trying to call with a flat list of options.

I think the first version of the patch was better:

    def changelv(vg, lvs, attrs, autobackup=False):

If you try to call with "--autobackup" or "-A", you lvm call will fail, and you
can easily fix your code to use the autobackup kwarg.

But I think that this will be better - making this change on all functions
using LVM_NOBACKUP:

    def changelv(vg, lvs, attrs, autobackup="n"):
        ...
        cmd.extend(("--autobackup", autobackup))

And remove LVM_NOBACKUP, so all functions behave in the same way.

In the long term, this code should have pythonic api using only kwargs
for lvm options, and must not support passing raw options:

    def changelv(vg, lv, autobackup="n", addtags=(), deltags=()):
       ...
       cmd.extend(("--autobackup", autobackup))
       for tag in addtags:
           cmd.extend(("--addtag", tag))
       ...      

We also need tests verifying that this change did not break the old
behavior. This should be easy by mocking _lvminfo while calling this
function, and checking that _lvminfo.cmd() and _lvminfo._invalidatelvs()
were called with the expected arguments.

What do you think?
Line 807:         cmd.extend(LVM_NOBACKUP)
Line 808:     if isinstance(attrs[0], str):
Line 809:         # ("--attribute", "value")
Line 810:         cmd.extend(attrs)


-- 
To view, visit https://gerrit.ovirt.org/38195
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I8190878fe071fad46800e666795b94779d745abf
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir <tjeyasin at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at gmail.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Sandro Bonazzola <sbonazzo at redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list