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

tjeyasin at redhat.com tjeyasin at redhat.com
Thu Mar 12 10:38:21 UTC 2015


Timothy Asir 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:
yes.  I will update the sentence
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 
yes. I will update the sentence
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:
Done
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?
Yes this change required in every functions. I will update the patch based on your suggestion.

in our gluster brick creation task, we will be using this without specifying this autobackup option like follows:
lvconvert --chunksize <dataalignment_val> --thinpool <poolname> --poolmetadata <vg/metadata_name> --poolmetadataspar n -y
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 wil
Sure, I agree your suggestion. Later we will enhance to accept kwargs for lvm options.
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: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini 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